From: Luis Henriques <lhenriques@suse.de> To: Vivek Goyal <vgoyal@redhat.com> Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, virtio-fs@redhat.com, miklos@szeredi.hu, dgilbert@redhat.com, seth.forshee@canonical.com Subject: Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2 Date: Mon, 29 Mar 2021 21:27:47 +0100 [thread overview] Message-ID: <87eefxk8n0.fsf@suse.de> (raw) In-Reply-To: <20210329182408.GE676525@redhat.com> (Vivek Goyal's message of "Mon, 29 Mar 2021 14:24:08 -0400") Vivek Goyal <vgoyal@redhat.com> writes: > On Mon, Mar 29, 2021 at 03:54:03PM +0100, Luis Henriques wrote: >> On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote: >> > Fuse client needs to send additional information to file server when >> > it calls SETXATTR(system.posix_acl_access). Right now there is no extra >> > space in fuse_setxattr_in. So introduce a v2 of the structure which has >> > more space in it and can be used to send extra flags. >> > >> > "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using >> > flag FUSE_SETXATTR_V2 during feature negotiations. >> > >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >> > --- >> > fs/fuse/acl.c | 2 +- >> > fs/fuse/fuse_i.h | 5 ++++- >> > fs/fuse/inode.c | 4 +++- >> > fs/fuse/xattr.c | 21 +++++++++++++++------ >> > include/uapi/linux/fuse.h | 10 ++++++++++ >> > 5 files changed, 33 insertions(+), 9 deletions(-) >> > >> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c >> > index e9c0f916349d..d31260a139d4 100644 >> > --- a/fs/fuse/acl.c >> > +++ b/fs/fuse/acl.c >> > @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, >> > return ret; >> > } >> > >> > - ret = fuse_setxattr(inode, name, value, size, 0); >> > + ret = fuse_setxattr(inode, name, value, size, 0, 0); >> > kfree(value); >> > } else { >> > ret = fuse_removexattr(inode, name); >> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h >> > index 63d97a15ffde..d00bf0b9a38c 100644 >> > --- a/fs/fuse/fuse_i.h >> > +++ b/fs/fuse/fuse_i.h >> > @@ -668,6 +668,9 @@ struct fuse_conn { >> > /** Is setxattr not implemented by fs? */ >> > unsigned no_setxattr:1; >> > >> > + /** Does file server support setxattr_v2 */ >> > + unsigned setxattr_v2:1; >> > + >> > /** Is getxattr not implemented by fs? */ >> > unsigned no_getxattr:1; >> > >> > @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked); >> > bool fuse_lock_inode(struct inode *inode); >> > >> > int fuse_setxattr(struct inode *inode, const char *name, const void *value, >> > - size_t size, int flags); >> > + size_t size, int flags, unsigned extra_flags); >> > ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value, >> > size_t size); >> > ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); >> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c >> > index b0e18b470e91..1c726df13f80 100644 >> > --- a/fs/fuse/inode.c >> > +++ b/fs/fuse/inode.c >> > @@ -1052,6 +1052,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, >> > fc->handle_killpriv_v2 = 1; >> > fm->sb->s_flags |= SB_NOSEC; >> > } >> > + if (arg->flags & FUSE_SETXATTR_V2) >> > + fc->setxattr_v2 = 1; >> > } else { >> > ra_pages = fc->max_read / PAGE_SIZE; >> > fc->no_lock = 1; >> > @@ -1095,7 +1097,7 @@ void fuse_send_init(struct fuse_mount *fm) >> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL | >> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS | >> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA | >> > - FUSE_HANDLE_KILLPRIV_V2; >> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_V2; >> > #ifdef CONFIG_FUSE_DAX >> > if (fm->fc->dax) >> > ia->in.flags |= FUSE_MAP_ALIGNMENT; >> > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c >> > index 1a7d7ace54e1..f2aae72653dc 100644 >> > --- a/fs/fuse/xattr.c >> > +++ b/fs/fuse/xattr.c >> > @@ -12,24 +12,33 @@ >> > #include <linux/posix_acl_xattr.h> >> > >> > int fuse_setxattr(struct inode *inode, const char *name, const void *value, >> > - size_t size, int flags) >> > + size_t size, int flags, unsigned extra_flags) >> > { >> > struct fuse_mount *fm = get_fuse_mount(inode); >> > FUSE_ARGS(args); >> > struct fuse_setxattr_in inarg; >> > + struct fuse_setxattr_in_v2 inarg_v2; >> > + bool setxattr_v2 = fm->fc->setxattr_v2; >> > int err; >> > >> > if (fm->fc->no_setxattr) >> > return -EOPNOTSUPP; >> > >> > memset(&inarg, 0, sizeof(inarg)); >> > - inarg.size = size; >> > - inarg.flags = flags; >> > + memset(&inarg_v2, 0, sizeof(inarg_v2)); >> > + if (setxattr_v2) { >> > + inarg_v2.size = size; >> > + inarg_v2.flags = flags; >> > + inarg_v2.setxattr_flags = extra_flags; >> > + } else { >> > + inarg.size = size; >> > + inarg.flags = flags; >> > + } >> > args.opcode = FUSE_SETXATTR; >> > args.nodeid = get_node_id(inode); >> > args.in_numargs = 3; >> > - args.in_args[0].size = sizeof(inarg); >> > - args.in_args[0].value = &inarg; >> > + args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg); >> > + args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg; >> >> And yet another minor: >> >> It's a bit awkward to have to cast '&inarg' to 'void *' just because >> you're using the ternary operator. Why not use an 'if' statement instead >> for initializing .size and .value? > > Yes, I had to use (void *), otherwise compiler was complaining about > returning different types of pointers. Interesting that compiler > expects to return same type of pointer. IIRC, K&R (which I unfortunately don't have at hand right now) says that the types of both expressions need to match, so probably a different compiler would show the same warning. Cheers, -- Luis > I think I am fine with this as well as adding explicit if statement. I > guess just a matter of taste. > > Miklos, what do you think? If you also prefer if statement instead, > I will make changes and post again. > > Vivek
WARNING: multiple messages have this Message-ID (diff)
From: Luis Henriques <lhenriques@suse.de> To: Vivek Goyal <vgoyal@redhat.com> Cc: miklos@szeredi.hu, linux-kernel@vger.kernel.org, virtio-fs@redhat.com, seth.forshee@canonical.com, linux-fsdevel@vger.kernel.org Subject: Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2 Date: Mon, 29 Mar 2021 21:27:47 +0100 [thread overview] Message-ID: <87eefxk8n0.fsf@suse.de> (raw) In-Reply-To: <20210329182408.GE676525@redhat.com> (Vivek Goyal's message of "Mon, 29 Mar 2021 14:24:08 -0400") Vivek Goyal <vgoyal@redhat.com> writes: > On Mon, Mar 29, 2021 at 03:54:03PM +0100, Luis Henriques wrote: >> On Thu, Mar 25, 2021 at 11:18:22AM -0400, Vivek Goyal wrote: >> > Fuse client needs to send additional information to file server when >> > it calls SETXATTR(system.posix_acl_access). Right now there is no extra >> > space in fuse_setxattr_in. So introduce a v2 of the structure which has >> > more space in it and can be used to send extra flags. >> > >> > "struct fuse_setxattr_in_v2" is only used if file server opts-in for it using >> > flag FUSE_SETXATTR_V2 during feature negotiations. >> > >> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> >> > --- >> > fs/fuse/acl.c | 2 +- >> > fs/fuse/fuse_i.h | 5 ++++- >> > fs/fuse/inode.c | 4 +++- >> > fs/fuse/xattr.c | 21 +++++++++++++++------ >> > include/uapi/linux/fuse.h | 10 ++++++++++ >> > 5 files changed, 33 insertions(+), 9 deletions(-) >> > >> > diff --git a/fs/fuse/acl.c b/fs/fuse/acl.c >> > index e9c0f916349d..d31260a139d4 100644 >> > --- a/fs/fuse/acl.c >> > +++ b/fs/fuse/acl.c >> > @@ -94,7 +94,7 @@ int fuse_set_acl(struct user_namespace *mnt_userns, struct inode *inode, >> > return ret; >> > } >> > >> > - ret = fuse_setxattr(inode, name, value, size, 0); >> > + ret = fuse_setxattr(inode, name, value, size, 0, 0); >> > kfree(value); >> > } else { >> > ret = fuse_removexattr(inode, name); >> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h >> > index 63d97a15ffde..d00bf0b9a38c 100644 >> > --- a/fs/fuse/fuse_i.h >> > +++ b/fs/fuse/fuse_i.h >> > @@ -668,6 +668,9 @@ struct fuse_conn { >> > /** Is setxattr not implemented by fs? */ >> > unsigned no_setxattr:1; >> > >> > + /** Does file server support setxattr_v2 */ >> > + unsigned setxattr_v2:1; >> > + >> > /** Is getxattr not implemented by fs? */ >> > unsigned no_getxattr:1; >> > >> > @@ -1170,7 +1173,7 @@ void fuse_unlock_inode(struct inode *inode, bool locked); >> > bool fuse_lock_inode(struct inode *inode); >> > >> > int fuse_setxattr(struct inode *inode, const char *name, const void *value, >> > - size_t size, int flags); >> > + size_t size, int flags, unsigned extra_flags); >> > ssize_t fuse_getxattr(struct inode *inode, const char *name, void *value, >> > size_t size); >> > ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); >> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c >> > index b0e18b470e91..1c726df13f80 100644 >> > --- a/fs/fuse/inode.c >> > +++ b/fs/fuse/inode.c >> > @@ -1052,6 +1052,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args, >> > fc->handle_killpriv_v2 = 1; >> > fm->sb->s_flags |= SB_NOSEC; >> > } >> > + if (arg->flags & FUSE_SETXATTR_V2) >> > + fc->setxattr_v2 = 1; >> > } else { >> > ra_pages = fc->max_read / PAGE_SIZE; >> > fc->no_lock = 1; >> > @@ -1095,7 +1097,7 @@ void fuse_send_init(struct fuse_mount *fm) >> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL | >> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS | >> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA | >> > - FUSE_HANDLE_KILLPRIV_V2; >> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_V2; >> > #ifdef CONFIG_FUSE_DAX >> > if (fm->fc->dax) >> > ia->in.flags |= FUSE_MAP_ALIGNMENT; >> > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c >> > index 1a7d7ace54e1..f2aae72653dc 100644 >> > --- a/fs/fuse/xattr.c >> > +++ b/fs/fuse/xattr.c >> > @@ -12,24 +12,33 @@ >> > #include <linux/posix_acl_xattr.h> >> > >> > int fuse_setxattr(struct inode *inode, const char *name, const void *value, >> > - size_t size, int flags) >> > + size_t size, int flags, unsigned extra_flags) >> > { >> > struct fuse_mount *fm = get_fuse_mount(inode); >> > FUSE_ARGS(args); >> > struct fuse_setxattr_in inarg; >> > + struct fuse_setxattr_in_v2 inarg_v2; >> > + bool setxattr_v2 = fm->fc->setxattr_v2; >> > int err; >> > >> > if (fm->fc->no_setxattr) >> > return -EOPNOTSUPP; >> > >> > memset(&inarg, 0, sizeof(inarg)); >> > - inarg.size = size; >> > - inarg.flags = flags; >> > + memset(&inarg_v2, 0, sizeof(inarg_v2)); >> > + if (setxattr_v2) { >> > + inarg_v2.size = size; >> > + inarg_v2.flags = flags; >> > + inarg_v2.setxattr_flags = extra_flags; >> > + } else { >> > + inarg.size = size; >> > + inarg.flags = flags; >> > + } >> > args.opcode = FUSE_SETXATTR; >> > args.nodeid = get_node_id(inode); >> > args.in_numargs = 3; >> > - args.in_args[0].size = sizeof(inarg); >> > - args.in_args[0].value = &inarg; >> > + args.in_args[0].size = setxattr_v2 ? sizeof(inarg_v2) : sizeof(inarg); >> > + args.in_args[0].value = setxattr_v2 ? &inarg_v2 : (void *)&inarg; >> >> And yet another minor: >> >> It's a bit awkward to have to cast '&inarg' to 'void *' just because >> you're using the ternary operator. Why not use an 'if' statement instead >> for initializing .size and .value? > > Yes, I had to use (void *), otherwise compiler was complaining about > returning different types of pointers. Interesting that compiler > expects to return same type of pointer. IIRC, K&R (which I unfortunately don't have at hand right now) says that the types of both expressions need to match, so probably a different compiler would show the same warning. Cheers, -- Luis > I think I am fine with this as well as adding explicit if statement. I > guess just a matter of taste. > > Miklos, what do you think? If you also prefer if statement instead, > I will make changes and post again. > > Vivek
next prev parent reply other threads:[~2021-03-29 20:28 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-25 15:18 [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set Vivek Goyal 2021-03-25 15:18 ` [Virtio-fs] " Vivek Goyal 2021-03-25 15:18 ` [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2 Vivek Goyal 2021-03-25 15:18 ` [Virtio-fs] " Vivek Goyal 2021-03-29 14:50 ` Luis Henriques 2021-03-29 14:50 ` [Virtio-fs] " Luis Henriques 2021-03-29 18:16 ` Vivek Goyal 2021-03-29 18:16 ` [Virtio-fs] " Vivek Goyal 2021-03-29 14:54 ` Luis Henriques 2021-03-29 14:54 ` [Virtio-fs] " Luis Henriques 2021-03-29 18:24 ` Vivek Goyal 2021-03-29 18:24 ` [Virtio-fs] " Vivek Goyal 2021-03-29 20:27 ` Luis Henriques [this message] 2021-03-29 20:27 ` Luis Henriques 2021-03-25 15:18 ` [PATCH v2 2/2] fuse: Add a flag FUSE_SETXATTR_ACL_KILL_SGID to kill SGID Vivek Goyal 2021-03-25 15:18 ` [Virtio-fs] " Vivek Goyal 2021-04-13 20:41 ` [Virtio-fs] [PATCH v2 0/2] fuse: Fix clearing SGID when access ACL is set Vivek Goyal 2021-04-14 11:57 ` Miklos Szeredi 2021-04-14 11:57 ` [Virtio-fs] " Miklos Szeredi 2021-04-14 12:58 ` Vivek Goyal 2021-04-14 12:58 ` [Virtio-fs] " Vivek Goyal 2021-06-17 14:35 ` Vivek Goyal 2021-06-17 14:35 ` [Virtio-fs] " Vivek Goyal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87eefxk8n0.fsf@suse.de \ --to=lhenriques@suse.de \ --cc=dgilbert@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=seth.forshee@canonical.com \ --cc=vgoyal@redhat.com \ --cc=virtio-fs@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.