* [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-25 15:18 ` Vivek Goyal
0 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2021-03-25 15:18 UTC (permalink / raw)
To: linux-fsdevel, linux-kernel, virtio-fs, miklos; +Cc: seth.forshee, vgoyal
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;
args.in_args[1].size = strlen(name) + 1;
args.in_args[1].value = name;
args.in_args[2].size = size;
@@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
if (!value)
return fuse_removexattr(inode, name);
- return fuse_setxattr(inode, name, value, size, flags);
+ return fuse_setxattr(inode, name, value, size, flags, 0);
}
static bool no_xattr_list(struct dentry *dentry)
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 54442612c48b..1bb555c1c117 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -179,6 +179,7 @@
* 7.33
* - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
* - add FUSE_OPEN_KILL_SUIDGID
+ * - add FUSE_SETXATTR_V2
*/
#ifndef _LINUX_FUSE_H
@@ -330,6 +331,7 @@ struct fuse_file_lock {
* does not have CAP_FSETID. Additionally upon
* write/truncate sgid is killed only if file has group
* execute permission. (Same as Linux VFS behavior).
+ * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
*/
#define FUSE_ASYNC_READ (1 << 0)
#define FUSE_POSIX_LOCKS (1 << 1)
@@ -360,6 +362,7 @@ struct fuse_file_lock {
#define FUSE_MAP_ALIGNMENT (1 << 26)
#define FUSE_SUBMOUNTS (1 << 27)
#define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
+#define FUSE_SETXATTR_V2 (1 << 29)
/**
* CUSE INIT request/reply flags
@@ -686,6 +689,13 @@ struct fuse_setxattr_in {
uint32_t flags;
};
+struct fuse_setxattr_in_v2 {
+ uint32_t size;
+ uint32_t flags;
+ uint32_t setxattr_flags;
+ uint32_t padding;
+};
+
struct fuse_getxattr_in {
uint32_t size;
uint32_t padding;
--
2.25.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
2021-03-25 15:18 ` [Virtio-fs] " Vivek Goyal
@ 2021-03-29 14:50 ` Luis Henriques
-1 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 14:50 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, linux-kernel, virtio-fs, miklos, dgilbert, seth.forshee
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;
> +
Minor (pedantic!) comment: most of the fields here start with 'no_*', so
maybe it's worth setting the logic to use 'no_setxattr_v2' instead?
Cheers,
--
Luís
> /** 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;
> args.in_args[1].size = strlen(name) + 1;
> args.in_args[1].value = name;
> args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> if (!value)
> return fuse_removexattr(inode, name);
>
> - return fuse_setxattr(inode, name, value, size, flags);
> + return fuse_setxattr(inode, name, value, size, flags, 0);
> }
>
> static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -179,6 +179,7 @@
> * 7.33
> * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> * - add FUSE_OPEN_KILL_SUIDGID
> + * - add FUSE_SETXATTR_V2
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ struct fuse_file_lock {
> * does not have CAP_FSETID. Additionally upon
> * write/truncate sgid is killed only if file has group
> * execute permission. (Same as Linux VFS behavior).
> + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> */
> #define FUSE_ASYNC_READ (1 << 0)
> #define FUSE_POSIX_LOCKS (1 << 1)
> @@ -360,6 +362,7 @@ struct fuse_file_lock {
> #define FUSE_MAP_ALIGNMENT (1 << 26)
> #define FUSE_SUBMOUNTS (1 << 27)
> #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> +#define FUSE_SETXATTR_V2 (1 << 29)
>
> /**
> * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> uint32_t flags;
> };
>
> +struct fuse_setxattr_in_v2 {
> + uint32_t size;
> + uint32_t flags;
> + uint32_t setxattr_flags;
> + uint32_t padding;
> +};
> +
> struct fuse_getxattr_in {
> uint32_t size;
> uint32_t padding;
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-29 14:50 ` Luis Henriques
0 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 14:50 UTC (permalink / raw)
To: Vivek Goyal; +Cc: miklos, linux-kernel, virtio-fs, seth.forshee, linux-fsdevel
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;
> +
Minor (pedantic!) comment: most of the fields here start with 'no_*', so
maybe it's worth setting the logic to use 'no_setxattr_v2' instead?
Cheers,
--
Luís
> /** 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;
> args.in_args[1].size = strlen(name) + 1;
> args.in_args[1].value = name;
> args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> if (!value)
> return fuse_removexattr(inode, name);
>
> - return fuse_setxattr(inode, name, value, size, flags);
> + return fuse_setxattr(inode, name, value, size, flags, 0);
> }
>
> static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -179,6 +179,7 @@
> * 7.33
> * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> * - add FUSE_OPEN_KILL_SUIDGID
> + * - add FUSE_SETXATTR_V2
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ struct fuse_file_lock {
> * does not have CAP_FSETID. Additionally upon
> * write/truncate sgid is killed only if file has group
> * execute permission. (Same as Linux VFS behavior).
> + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> */
> #define FUSE_ASYNC_READ (1 << 0)
> #define FUSE_POSIX_LOCKS (1 << 1)
> @@ -360,6 +362,7 @@ struct fuse_file_lock {
> #define FUSE_MAP_ALIGNMENT (1 << 26)
> #define FUSE_SUBMOUNTS (1 << 27)
> #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> +#define FUSE_SETXATTR_V2 (1 << 29)
>
> /**
> * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> uint32_t flags;
> };
>
> +struct fuse_setxattr_in_v2 {
> + uint32_t size;
> + uint32_t flags;
> + uint32_t setxattr_flags;
> + uint32_t padding;
> +};
> +
> struct fuse_getxattr_in {
> uint32_t size;
> uint32_t padding;
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
2021-03-29 14:50 ` [Virtio-fs] " Luis Henriques
@ 2021-03-29 18:16 ` Vivek Goyal
-1 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2021-03-29 18:16 UTC (permalink / raw)
To: Luis Henriques
Cc: linux-fsdevel, linux-kernel, virtio-fs, miklos, dgilbert, seth.forshee
On Mon, Mar 29, 2021 at 03:50:37PM +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;
> > +
>
> Minor (pedantic!) comment: most of the fields here start with 'no_*', so
> maybe it's worth setting the logic to use 'no_setxattr_v2' instead?
Hi Luis,
"setxattr_v2" kind of makes more sense to me because it is disabled
by default untile and unless client opts in. If I use no_setxattr_v2,
then it means by default I will have to initialize it to 1. Right
now, following automatically takes care of it.
fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
Also, there are other examples which don't use "no_" prefix.
auto_inval_data, explicit_inval_data, do_readdirplus, readdirplus_auto,
async_dio..... and list goes on.
Vivek
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-29 18:16 ` Vivek Goyal
0 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2021-03-29 18:16 UTC (permalink / raw)
To: Luis Henriques
Cc: miklos, linux-kernel, virtio-fs, seth.forshee, linux-fsdevel
On Mon, Mar 29, 2021 at 03:50:37PM +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;
> > +
>
> Minor (pedantic!) comment: most of the fields here start with 'no_*', so
> maybe it's worth setting the logic to use 'no_setxattr_v2' instead?
Hi Luis,
"setxattr_v2" kind of makes more sense to me because it is disabled
by default untile and unless client opts in. If I use no_setxattr_v2,
then it means by default I will have to initialize it to 1. Right
now, following automatically takes care of it.
fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
Also, there are other examples which don't use "no_" prefix.
auto_inval_data, explicit_inval_data, do_readdirplus, readdirplus_auto,
async_dio..... and list goes on.
Vivek
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
2021-03-25 15:18 ` [Virtio-fs] " Vivek Goyal
@ 2021-03-29 14:54 ` Luis Henriques
-1 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 14:54 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, linux-kernel, virtio-fs, miklos, dgilbert, seth.forshee
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?
Cheers,
--
Luís
> args.in_args[1].size = strlen(name) + 1;
> args.in_args[1].value = name;
> args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> if (!value)
> return fuse_removexattr(inode, name);
>
> - return fuse_setxattr(inode, name, value, size, flags);
> + return fuse_setxattr(inode, name, value, size, flags, 0);
> }
>
> static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -179,6 +179,7 @@
> * 7.33
> * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> * - add FUSE_OPEN_KILL_SUIDGID
> + * - add FUSE_SETXATTR_V2
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ struct fuse_file_lock {
> * does not have CAP_FSETID. Additionally upon
> * write/truncate sgid is killed only if file has group
> * execute permission. (Same as Linux VFS behavior).
> + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> */
> #define FUSE_ASYNC_READ (1 << 0)
> #define FUSE_POSIX_LOCKS (1 << 1)
> @@ -360,6 +362,7 @@ struct fuse_file_lock {
> #define FUSE_MAP_ALIGNMENT (1 << 26)
> #define FUSE_SUBMOUNTS (1 << 27)
> #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> +#define FUSE_SETXATTR_V2 (1 << 29)
>
> /**
> * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> uint32_t flags;
> };
>
> +struct fuse_setxattr_in_v2 {
> + uint32_t size;
> + uint32_t flags;
> + uint32_t setxattr_flags;
> + uint32_t padding;
> +};
> +
> struct fuse_getxattr_in {
> uint32_t size;
> uint32_t padding;
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-29 14:54 ` Luis Henriques
0 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 14:54 UTC (permalink / raw)
To: Vivek Goyal; +Cc: miklos, linux-kernel, virtio-fs, seth.forshee, linux-fsdevel
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?
Cheers,
--
Luís
> args.in_args[1].size = strlen(name) + 1;
> args.in_args[1].value = name;
> args.in_args[2].size = size;
> @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> if (!value)
> return fuse_removexattr(inode, name);
>
> - return fuse_setxattr(inode, name, value, size, flags);
> + return fuse_setxattr(inode, name, value, size, flags, 0);
> }
>
> static bool no_xattr_list(struct dentry *dentry)
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 54442612c48b..1bb555c1c117 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -179,6 +179,7 @@
> * 7.33
> * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> * - add FUSE_OPEN_KILL_SUIDGID
> + * - add FUSE_SETXATTR_V2
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -330,6 +331,7 @@ struct fuse_file_lock {
> * does not have CAP_FSETID. Additionally upon
> * write/truncate sgid is killed only if file has group
> * execute permission. (Same as Linux VFS behavior).
> + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> */
> #define FUSE_ASYNC_READ (1 << 0)
> #define FUSE_POSIX_LOCKS (1 << 1)
> @@ -360,6 +362,7 @@ struct fuse_file_lock {
> #define FUSE_MAP_ALIGNMENT (1 << 26)
> #define FUSE_SUBMOUNTS (1 << 27)
> #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> +#define FUSE_SETXATTR_V2 (1 << 29)
>
> /**
> * CUSE INIT request/reply flags
> @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> uint32_t flags;
> };
>
> +struct fuse_setxattr_in_v2 {
> + uint32_t size;
> + uint32_t flags;
> + uint32_t setxattr_flags;
> + uint32_t padding;
> +};
> +
> struct fuse_getxattr_in {
> uint32_t size;
> uint32_t padding;
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
2021-03-29 14:54 ` [Virtio-fs] " Luis Henriques
@ 2021-03-29 18:24 ` Vivek Goyal
-1 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2021-03-29 18:24 UTC (permalink / raw)
To: Luis Henriques
Cc: linux-fsdevel, linux-kernel, virtio-fs, miklos, dgilbert, seth.forshee
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.
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
>
> Cheers,
> --
> Luís
>
> > args.in_args[1].size = strlen(name) + 1;
> > args.in_args[1].value = name;
> > args.in_args[2].size = size;
> > @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> > if (!value)
> > return fuse_removexattr(inode, name);
> >
> > - return fuse_setxattr(inode, name, value, size, flags);
> > + return fuse_setxattr(inode, name, value, size, flags, 0);
> > }
> >
> > static bool no_xattr_list(struct dentry *dentry)
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 54442612c48b..1bb555c1c117 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -179,6 +179,7 @@
> > * 7.33
> > * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> > * - add FUSE_OPEN_KILL_SUIDGID
> > + * - add FUSE_SETXATTR_V2
> > */
> >
> > #ifndef _LINUX_FUSE_H
> > @@ -330,6 +331,7 @@ struct fuse_file_lock {
> > * does not have CAP_FSETID. Additionally upon
> > * write/truncate sgid is killed only if file has group
> > * execute permission. (Same as Linux VFS behavior).
> > + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> > */
> > #define FUSE_ASYNC_READ (1 << 0)
> > #define FUSE_POSIX_LOCKS (1 << 1)
> > @@ -360,6 +362,7 @@ struct fuse_file_lock {
> > #define FUSE_MAP_ALIGNMENT (1 << 26)
> > #define FUSE_SUBMOUNTS (1 << 27)
> > #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> > +#define FUSE_SETXATTR_V2 (1 << 29)
> >
> > /**
> > * CUSE INIT request/reply flags
> > @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> > uint32_t flags;
> > };
> >
> > +struct fuse_setxattr_in_v2 {
> > + uint32_t size;
> > + uint32_t flags;
> > + uint32_t setxattr_flags;
> > + uint32_t padding;
> > +};
> > +
> > struct fuse_getxattr_in {
> > uint32_t size;
> > uint32_t padding;
> > --
> > 2.25.4
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-29 18:24 ` Vivek Goyal
0 siblings, 0 replies; 23+ messages in thread
From: Vivek Goyal @ 2021-03-29 18:24 UTC (permalink / raw)
To: Luis Henriques
Cc: miklos, linux-kernel, virtio-fs, seth.forshee, linux-fsdevel
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.
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
>
> Cheers,
> --
> Luís
>
> > args.in_args[1].size = strlen(name) + 1;
> > args.in_args[1].value = name;
> > args.in_args[2].size = size;
> > @@ -199,7 +208,7 @@ static int fuse_xattr_set(const struct xattr_handler *handler,
> > if (!value)
> > return fuse_removexattr(inode, name);
> >
> > - return fuse_setxattr(inode, name, value, size, flags);
> > + return fuse_setxattr(inode, name, value, size, flags, 0);
> > }
> >
> > static bool no_xattr_list(struct dentry *dentry)
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 54442612c48b..1bb555c1c117 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -179,6 +179,7 @@
> > * 7.33
> > * - add FUSE_HANDLE_KILLPRIV_V2, FUSE_WRITE_KILL_SUIDGID, FATTR_KILL_SUIDGID
> > * - add FUSE_OPEN_KILL_SUIDGID
> > + * - add FUSE_SETXATTR_V2
> > */
> >
> > #ifndef _LINUX_FUSE_H
> > @@ -330,6 +331,7 @@ struct fuse_file_lock {
> > * does not have CAP_FSETID. Additionally upon
> > * write/truncate sgid is killed only if file has group
> > * execute permission. (Same as Linux VFS behavior).
> > + * FUSE_SETXATTR_V2: Does file server support V2 of struct fuse_setxattr_in
> > */
> > #define FUSE_ASYNC_READ (1 << 0)
> > #define FUSE_POSIX_LOCKS (1 << 1)
> > @@ -360,6 +362,7 @@ struct fuse_file_lock {
> > #define FUSE_MAP_ALIGNMENT (1 << 26)
> > #define FUSE_SUBMOUNTS (1 << 27)
> > #define FUSE_HANDLE_KILLPRIV_V2 (1 << 28)
> > +#define FUSE_SETXATTR_V2 (1 << 29)
> >
> > /**
> > * CUSE INIT request/reply flags
> > @@ -686,6 +689,13 @@ struct fuse_setxattr_in {
> > uint32_t flags;
> > };
> >
> > +struct fuse_setxattr_in_v2 {
> > + uint32_t size;
> > + uint32_t flags;
> > + uint32_t setxattr_flags;
> > + uint32_t padding;
> > +};
> > +
> > struct fuse_getxattr_in {
> > uint32_t size;
> > uint32_t padding;
> > --
> > 2.25.4
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
2021-03-29 18:24 ` [Virtio-fs] " Vivek Goyal
@ 2021-03-29 20:27 ` Luis Henriques
-1 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 20:27 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, linux-kernel, virtio-fs, miklos, dgilbert, seth.forshee
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Virtio-fs] [PATCH v2 1/2] fuse: Add support for FUSE_SETXATTR_V2
@ 2021-03-29 20:27 ` Luis Henriques
0 siblings, 0 replies; 23+ messages in thread
From: Luis Henriques @ 2021-03-29 20:27 UTC (permalink / raw)
To: Vivek Goyal; +Cc: miklos, linux-kernel, virtio-fs, seth.forshee, linux-fsdevel
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
^ permalink raw reply [flat|nested] 23+ messages in thread