Messages in this thread | | | Date | Fri, 1 Jul 2022 18:22:09 -0600 | Subject | Re: [PATCH v2 3/9] mm/mshare: make msharefs writable and support directories | From | Khalid Aziz <> |
| |
On 6/30/22 17:09, Al Viro wrote: > On Wed, Jun 29, 2022 at 04:53:54PM -0600, Khalid Aziz wrote: > >> +static int >> +msharefs_open(struct inode *inode, struct file *file) >> +{ >> + return simple_open(inode, file); >> +} > > Again, whatever for? > >> +static struct dentry >> +*msharefs_alloc_dentry(struct dentry *parent, const char *name) >> +{ >> + struct dentry *d; >> + struct qstr q; >> + int err; >> + >> + q.name = name; >> + q.len = strlen(name); >> + >> + err = msharefs_d_hash(parent, &q); >> + if (err) >> + return ERR_PTR(err); >> + >> + d = d_alloc(parent, &q); >> + if (d) >> + return d; >> + >> + return ERR_PTR(-ENOMEM); >> +} > > And it's different from d_alloc_name() how, exactly?
By making minor changes to my other code, I was able to use all of the standard functions you pointed out. That simplified my patch quite a bit. Thank you!
> >> + case S_IFLNK: >> + inode->i_op = &page_symlink_inode_operations; >> + break; > > Really? You've got symlinks here?
I intended to support symlinks on msharefs but I am not sure if I see a use case at this time. I can drop support for symlinks and add it in future if there is a use case.
> >> + default: >> + discard_new_inode(inode); >> + inode = NULL; > > That's an odd way to spell BUG()...
I think what you are saying is this default case represents a bug and I should report it as such. Is that right, or should I not have a default case at all (which is what I am seeing in some of the other places)?
> >> +static int >> +msharefs_mknod(struct user_namespace *mnt_userns, struct inode *dir, >> + struct dentry *dentry, umode_t mode, dev_t dev) >> +{ >> + struct inode *inode; >> + int err = 0; >> + >> + inode = msharefs_get_inode(dir->i_sb, dir, mode); >> + if (IS_ERR(inode)) >> + return PTR_ERR(inode); >> + >> + d_instantiate(dentry, inode); >> + dget(dentry); >> + dir->i_mtime = dir->i_ctime = current_time(dir); >> + >> + return err; >> +} > > BTW, what's the point of having device nodes on that thing?
There will be no device nodes on msharefs. Are you referring to the dev_t parameter in msharefs_mknod() declaration? If so, I am following the prototype declaration for that function from fs.h:
int (*mknod) (struct user_namespace *, struct inode *,struct dentry *, umode_t,dev_t);
If I am misunderstanding, please correct me.
> >> +static int >> +msharefs_create(struct user_namespace *mnt_userns, struct inode *dir, >> + struct dentry *dentry, umode_t mode, bool excl) >> +{ >> + return msharefs_mknod(&init_user_ns, dir, dentry, mode | S_IFREG, 0); >> +} >> + >> +static int >> +msharefs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> + struct dentry *dentry, umode_t mode) >> +{ >> + int ret = msharefs_mknod(&init_user_ns, dir, dentry, mode | S_IFDIR, 0); >> + >> + if (!ret) >> + inc_nlink(dir); >> + return ret; >> +} >> + >> +static const struct inode_operations msharefs_file_inode_ops = { >> + .setattr = simple_setattr, >> + .getattr = simple_getattr, >> +}; >> +static const struct inode_operations msharefs_dir_inode_ops = { >> + .create = msharefs_create, >> + .lookup = simple_lookup, >> + .link = simple_link, >> + .unlink = simple_unlink, >> + .mkdir = msharefs_mkdir, >> + .rmdir = simple_rmdir, >> + .mknod = msharefs_mknod, >> + .rename = simple_rename, >> +}; >> + >> static void >> mshare_evict_inode(struct inode *inode) >> { >> @@ -58,7 +175,7 @@ mshare_info_read(struct file *file, char __user *buf, size_t nbytes, >> { >> char s[80]; >> >> - sprintf(s, "%ld", PGDIR_SIZE); >> + sprintf(s, "%ld\n", PGDIR_SIZE); >> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s)); >> } >> >> @@ -72,6 +189,38 @@ static const struct super_operations mshare_s_ops = { >> .evict_inode = mshare_evict_inode, >> }; >> >> +static int >> +prepopulate_files(struct super_block *s, struct inode *dir, >> + struct dentry *root, const struct tree_descr *files) >> +{ >> + int i; >> + struct inode *inode; >> + struct dentry *dentry; >> + >> + for (i = 0; !files->name || files->name[0]; i++, files++) { >> + if (!files->name) >> + continue; >> + >> + dentry = msharefs_alloc_dentry(root, files->name); >> + if (!dentry) >> + return -ENOMEM; >> + >> + inode = msharefs_get_inode(s, dir, S_IFREG | files->mode); >> + if (!inode) { >> + dput(dentry); >> + return -ENOMEM; >> + } >> + inode->i_mode = S_IFREG | files->mode; >> + inode->i_atime = inode->i_mtime = inode->i_ctime >> + = current_time(inode); >> + inode->i_fop = files->ops; >> + inode->i_ino = i; >> + d_add(dentry, inode); >> + } >> + >> + return 0; >> +} > > Looks remarkably similar to something I've seen somewhere... fs/libfs.c, > if I'm not mistaken... > > Sarcasm aside, what's wrong with using simple_fill_super()? I started out using simple_fill_super() in patch 1. I found that when I use simple_fill_super(), I end up with a filesystem that userspace can not create a file in. I looked at other code like shmfs and efivarfs and wrote similar code which got me a writable filesystem. I might be missing something basic and if there is a way to use simple_fill_super() and be able to support file creation from userspace, I would much rather use simple_fill_super().
Thanks, Khalid
| |