From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f68.google.com ([209.85.218.68]:33712 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753914AbcHDLVf (ORCPT ); Thu, 4 Aug 2016 07:21:35 -0400 Received: by mail-oi0-f68.google.com with SMTP id l9so24356845oih.0 for ; Thu, 04 Aug 2016 04:21:35 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1470086846-19844-2-git-send-email-seth.forshee@canonical.com> References: <1470086846-19844-1-git-send-email-seth.forshee@canonical.com> <1470086846-19844-2-git-send-email-seth.forshee@canonical.com> From: Miklos Szeredi Date: Thu, 4 Aug 2016 13:09:58 +0200 Message-ID: Subject: Re: [RFC v3 1/2] fuse: Use generic xattr ops To: Seth Forshee Cc: fuse-devel , linux-fsdevel@vger.kernel.org, "Eric W. Biederman" , Michael j Theall , =?UTF-8?B?SmVhbi1QaWVycmUgQW5kcsOp?= , Nikolaus Rath Content-Type: text/plain; charset=UTF-8 Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Aug 1, 2016 at 11:27 PM, Seth Forshee wrote: > In preparation for posix acl support, rework fuse to use xattr > handlers and the generic setxattr/getxattr/listxattr callbacks. > Split the xattr code out into it's own file, and promote symbols > to module-global scope as needed. > > Functionally these changes have no impact, as fuse still uses a > single handler for all xattrs which uses the old callbacks. One comment at the very bottom of the patch. Otherwise it looks okay. Thanks, Miklos > > Signed-off-by: Seth Forshee > --- > fs/fuse/Makefile | 2 +- > fs/fuse/dir.c | 167 ++++---------------------------------------------- > fs/fuse/fuse_i.h | 5 ++ > fs/fuse/inode.c | 1 + > fs/fuse/xattr.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 201 insertions(+), 157 deletions(-) > create mode 100644 fs/fuse/xattr.c > > diff --git a/fs/fuse/Makefile b/fs/fuse/Makefile > index e95eeb445e58..448aa27ada00 100644 > --- a/fs/fuse/Makefile > +++ b/fs/fuse/Makefile > @@ -5,4 +5,4 @@ > obj-$(CONFIG_FUSE_FS) += fuse.o > obj-$(CONFIG_CUSE) += cuse.o > > -fuse-objs := dev.o dir.o file.o inode.o control.o > +fuse-objs := dev.o dir.o file.o inode.o control.o xattr.o > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c > index 8466e122ee62..9df55b8e776a 100644 > --- a/fs/fuse/dir.c > +++ b/fs/fuse/dir.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > > static bool fuse_use_readdirplus(struct inode *dir, struct dir_context *ctx) > { > @@ -632,7 +633,7 @@ static int fuse_symlink(struct inode *dir, struct dentry *entry, > return create_new_entry(fc, &args, dir, entry, S_IFLNK); > } > > -static inline void fuse_update_ctime(struct inode *inode) > +void fuse_update_ctime(struct inode *inode) > { > if (!IS_NOCMTIME(inode)) { > inode->i_ctime = current_fs_time(inode->i_sb); > @@ -1719,152 +1720,6 @@ static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry, > return fuse_update_attributes(inode, stat, NULL, NULL); > } > > -static int fuse_setxattr(struct dentry *unused, struct inode *inode, > - const char *name, const void *value, > - size_t size, int flags) > -{ > - struct fuse_conn *fc = get_fuse_conn(inode); > - FUSE_ARGS(args); > - struct fuse_setxattr_in inarg; > - int err; > - > - if (fc->no_setxattr) > - return -EOPNOTSUPP; > - > - memset(&inarg, 0, sizeof(inarg)); > - inarg.size = size; > - inarg.flags = flags; > - args.in.h.opcode = FUSE_SETXATTR; > - args.in.h.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[1].size = strlen(name) + 1; > - args.in.args[1].value = name; > - args.in.args[2].size = size; > - args.in.args[2].value = value; > - err = fuse_simple_request(fc, &args); > - if (err == -ENOSYS) { > - fc->no_setxattr = 1; > - err = -EOPNOTSUPP; > - } > - if (!err) { > - fuse_invalidate_attr(inode); > - fuse_update_ctime(inode); > - } > - return err; > -} > - > -static ssize_t fuse_getxattr(struct dentry *entry, struct inode *inode, > - const char *name, void *value, size_t size) > -{ > - struct fuse_conn *fc = get_fuse_conn(inode); > - FUSE_ARGS(args); > - struct fuse_getxattr_in inarg; > - struct fuse_getxattr_out outarg; > - ssize_t ret; > - > - if (fc->no_getxattr) > - return -EOPNOTSUPP; > - > - memset(&inarg, 0, sizeof(inarg)); > - inarg.size = size; > - args.in.h.opcode = FUSE_GETXATTR; > - args.in.h.nodeid = get_node_id(inode); > - args.in.numargs = 2; > - args.in.args[0].size = sizeof(inarg); > - args.in.args[0].value = &inarg; > - args.in.args[1].size = strlen(name) + 1; > - args.in.args[1].value = name; > - /* This is really two different operations rolled into one */ > - args.out.numargs = 1; > - if (size) { > - args.out.argvar = 1; > - args.out.args[0].size = size; > - args.out.args[0].value = value; > - } else { > - args.out.args[0].size = sizeof(outarg); > - args.out.args[0].value = &outarg; > - } > - ret = fuse_simple_request(fc, &args); > - if (!ret && !size) > - ret = outarg.size; > - if (ret == -ENOSYS) { > - fc->no_getxattr = 1; > - ret = -EOPNOTSUPP; > - } > - return ret; > -} > - > -static ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size) > -{ > - struct inode *inode = d_inode(entry); > - struct fuse_conn *fc = get_fuse_conn(inode); > - FUSE_ARGS(args); > - struct fuse_getxattr_in inarg; > - struct fuse_getxattr_out outarg; > - ssize_t ret; > - > - if (!fuse_allow_current_process(fc)) > - return -EACCES; > - > - if (fc->no_listxattr) > - return -EOPNOTSUPP; > - > - memset(&inarg, 0, sizeof(inarg)); > - inarg.size = size; > - args.in.h.opcode = FUSE_LISTXATTR; > - args.in.h.nodeid = get_node_id(inode); > - args.in.numargs = 1; > - args.in.args[0].size = sizeof(inarg); > - args.in.args[0].value = &inarg; > - /* This is really two different operations rolled into one */ > - args.out.numargs = 1; > - if (size) { > - args.out.argvar = 1; > - args.out.args[0].size = size; > - args.out.args[0].value = list; > - } else { > - args.out.args[0].size = sizeof(outarg); > - args.out.args[0].value = &outarg; > - } > - ret = fuse_simple_request(fc, &args); > - if (!ret && !size) > - ret = outarg.size; > - if (ret == -ENOSYS) { > - fc->no_listxattr = 1; > - ret = -EOPNOTSUPP; > - } > - return ret; > -} > - > -static int fuse_removexattr(struct dentry *entry, const char *name) > -{ > - struct inode *inode = d_inode(entry); > - struct fuse_conn *fc = get_fuse_conn(inode); > - FUSE_ARGS(args); > - int err; > - > - if (fc->no_removexattr) > - return -EOPNOTSUPP; > - > - args.in.h.opcode = FUSE_REMOVEXATTR; > - args.in.h.nodeid = get_node_id(inode); > - args.in.numargs = 1; > - args.in.args[0].size = strlen(name) + 1; > - args.in.args[0].value = name; > - err = fuse_simple_request(fc, &args); > - if (err == -ENOSYS) { > - fc->no_removexattr = 1; > - err = -EOPNOTSUPP; > - } > - if (!err) { > - fuse_invalidate_attr(inode); > - fuse_update_ctime(inode); > - } > - return err; > -} > - > static const struct inode_operations fuse_dir_inode_operations = { > .lookup = fuse_lookup, > .mkdir = fuse_mkdir, > @@ -1879,10 +1734,10 @@ static const struct inode_operations fuse_dir_inode_operations = { > .mknod = fuse_mknod, > .permission = fuse_permission, > .getattr = fuse_getattr, > - .setxattr = fuse_setxattr, > - .getxattr = fuse_getxattr, > + .setxattr = generic_setxattr, > + .getxattr = generic_getxattr, > .listxattr = fuse_listxattr, > - .removexattr = fuse_removexattr, > + .removexattr = generic_removexattr, > }; > > static const struct file_operations fuse_dir_operations = { > @@ -1900,10 +1755,10 @@ static const struct inode_operations fuse_common_inode_operations = { > .setattr = fuse_setattr, > .permission = fuse_permission, > .getattr = fuse_getattr, > - .setxattr = fuse_setxattr, > - .getxattr = fuse_getxattr, > + .setxattr = generic_setxattr, > + .getxattr = generic_getxattr, > .listxattr = fuse_listxattr, > - .removexattr = fuse_removexattr, > + .removexattr = generic_removexattr, > }; > > static const struct inode_operations fuse_symlink_inode_operations = { > @@ -1911,10 +1766,10 @@ static const struct inode_operations fuse_symlink_inode_operations = { > .get_link = fuse_get_link, > .readlink = generic_readlink, > .getattr = fuse_getattr, > - .setxattr = fuse_setxattr, > - .getxattr = fuse_getxattr, > + .setxattr = generic_setxattr, > + .getxattr = generic_getxattr, > .listxattr = fuse_listxattr, > - .removexattr = fuse_removexattr, > + .removexattr = generic_removexattr, > }; > > void fuse_init_common(struct inode *inode) > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index 9f4c3c82edd6..93a5e8191da1 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -903,6 +903,8 @@ int fuse_allow_current_process(struct fuse_conn *fc); > > u64 fuse_lock_owner_id(struct fuse_conn *fc, fl_owner_t id); > > +void fuse_update_ctime(struct inode *inode); > + > int fuse_update_attributes(struct inode *inode, struct kstat *stat, > struct file *file, bool *refreshed); > > @@ -964,4 +966,7 @@ int fuse_do_setattr(struct inode *inode, struct iattr *attr, > > void fuse_set_initialized(struct fuse_conn *fc); > > +ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size); > +extern const struct xattr_handler *fuse_xattr_handlers[]; > + > #endif /* _FS_FUSE_I_H */ > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c > index 254f1944ee98..fee06e48157d 100644 > --- a/fs/fuse/inode.c > +++ b/fs/fuse/inode.c > @@ -1066,6 +1066,7 @@ static int fuse_fill_super(struct super_block *sb, void *data, int silent) > } > sb->s_magic = FUSE_SUPER_MAGIC; > sb->s_op = &fuse_super_operations; > + sb->s_xattr = fuse_xattr_handlers; > sb->s_maxbytes = MAX_LFS_FILESIZE; > sb->s_time_gran = 1; > sb->s_export_op = &fuse_export_operations; > diff --git a/fs/fuse/xattr.c b/fs/fuse/xattr.c > new file mode 100644 > index 000000000000..d30e99610217 > --- /dev/null > +++ b/fs/fuse/xattr.c > @@ -0,0 +1,183 @@ > +/* > + FUSE: Filesystem in Userspace > + Copyright (C) 2001-2008 Miklos Szeredi > + > + This program can be distributed under the terms of the GNU GPL. > + See the file COPYING. > +*/ > + > +#include "fuse_i.h" > + > +#include > + > +static int fuse_setxattr(struct inode *inode, const char *name, > + const void *value, size_t size, int flags) > +{ > + struct fuse_conn *fc = get_fuse_conn(inode); > + FUSE_ARGS(args); > + struct fuse_setxattr_in inarg; > + int err; > + > + if (fc->no_setxattr) > + return -EOPNOTSUPP; > + > + memset(&inarg, 0, sizeof(inarg)); > + inarg.size = size; > + inarg.flags = flags; > + args.in.h.opcode = FUSE_SETXATTR; > + args.in.h.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[1].size = strlen(name) + 1; > + args.in.args[1].value = name; > + args.in.args[2].size = size; > + args.in.args[2].value = value; > + err = fuse_simple_request(fc, &args); > + if (err == -ENOSYS) { > + fc->no_setxattr = 1; > + err = -EOPNOTSUPP; > + } > + if (!err) { > + fuse_invalidate_attr(inode); > + fuse_update_ctime(inode); > + } > + return err; > +} > + > +static ssize_t fuse_getxattr(struct inode *inode, const char *name, > + void *value, size_t size) > +{ > + struct fuse_conn *fc = get_fuse_conn(inode); > + FUSE_ARGS(args); > + struct fuse_getxattr_in inarg; > + struct fuse_getxattr_out outarg; > + ssize_t ret; > + > + if (fc->no_getxattr) > + return -EOPNOTSUPP; > + > + memset(&inarg, 0, sizeof(inarg)); > + inarg.size = size; > + args.in.h.opcode = FUSE_GETXATTR; > + args.in.h.nodeid = get_node_id(inode); > + args.in.numargs = 2; > + args.in.args[0].size = sizeof(inarg); > + args.in.args[0].value = &inarg; > + args.in.args[1].size = strlen(name) + 1; > + args.in.args[1].value = name; > + /* This is really two different operations rolled into one */ > + args.out.numargs = 1; > + if (size) { > + args.out.argvar = 1; > + args.out.args[0].size = size; > + args.out.args[0].value = value; > + } else { > + args.out.args[0].size = sizeof(outarg); > + args.out.args[0].value = &outarg; > + } > + ret = fuse_simple_request(fc, &args); > + if (!ret && !size) > + ret = outarg.size; > + if (ret == -ENOSYS) { > + fc->no_getxattr = 1; > + ret = -EOPNOTSUPP; > + } > + return ret; > +} > + > +ssize_t fuse_listxattr(struct dentry *entry, char *list, size_t size) > +{ > + struct inode *inode = d_inode(entry); > + struct fuse_conn *fc = get_fuse_conn(inode); > + FUSE_ARGS(args); > + struct fuse_getxattr_in inarg; > + struct fuse_getxattr_out outarg; > + ssize_t ret; > + > + if (!fuse_allow_current_process(fc)) > + return -EACCES; > + > + if (fc->no_listxattr) > + return -EOPNOTSUPP; > + > + memset(&inarg, 0, sizeof(inarg)); > + inarg.size = size; > + args.in.h.opcode = FUSE_LISTXATTR; > + args.in.h.nodeid = get_node_id(inode); > + args.in.numargs = 1; > + args.in.args[0].size = sizeof(inarg); > + args.in.args[0].value = &inarg; > + /* This is really two different operations rolled into one */ > + args.out.numargs = 1; > + if (size) { > + args.out.argvar = 1; > + args.out.args[0].size = size; > + args.out.args[0].value = list; > + } else { > + args.out.args[0].size = sizeof(outarg); > + args.out.args[0].value = &outarg; > + } > + ret = fuse_simple_request(fc, &args); > + if (!ret && !size) > + ret = outarg.size; > + if (ret == -ENOSYS) { > + fc->no_listxattr = 1; > + ret = -EOPNOTSUPP; > + } > + return ret; > +} > + > +static int fuse_removexattr(struct inode *inode, const char *name) > +{ > + struct fuse_conn *fc = get_fuse_conn(inode); > + FUSE_ARGS(args); > + int err; > + > + if (fc->no_removexattr) > + return -EOPNOTSUPP; > + > + args.in.h.opcode = FUSE_REMOVEXATTR; > + args.in.h.nodeid = get_node_id(inode); > + args.in.numargs = 1; > + args.in.args[0].size = strlen(name) + 1; > + args.in.args[0].value = name; > + err = fuse_simple_request(fc, &args); > + if (err == -ENOSYS) { > + fc->no_removexattr = 1; > + err = -EOPNOTSUPP; > + } > + if (!err) { > + fuse_invalidate_attr(inode); > + fuse_update_ctime(inode); > + } > + return err; > +} > + > +static int fuse_xattr_get(const struct xattr_handler *handler, > + struct dentry *dentry, struct inode *inode, > + const char *name, void *value, size_t size) > +{ > + return fuse_getxattr(inode, name, value, size); > +} > + > +static int fuse_xattr_set(const struct xattr_handler *handler, > + struct dentry *dentry, struct inode *inode, > + const char *name, const void *value, size_t size, > + int flags) > +{ > + if (!value) > + return fuse_removexattr(inode, name); > + > + return fuse_setxattr(inode, name, value, size, flags); > +} > + > +static const struct xattr_handler fuse_xattr_handler = { > + .prefix = "", > + .get = fuse_xattr_get, > + .set = fuse_xattr_set, > +}; > + > +const struct xattr_handler *fuse_xattr_handlers[] = { > + &fuse_xattr_handler, Please terminate array with NULL, despite the catch-all prefix. The less subtle the better. Thanks, Miklos