| Message ID | 1551414895-22925-10-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc props.c cleanups | expand |
On Fri, Mar 01, 2019 at 12:34:55PM +0800, Anand Jain wrote: > Now btrfs_setxattr() is a very small function with just check for > readonly FS and redirect the call to do_setxattr(). So instead > move that checks to the parent functions and call do_setxattr() > directly. Delete original btrfs_setxattr(), and rename do_setxattr() > to btrfs_setxattr(). Also add few c-style. Kindly note the arguments > of original do_setxattr() and original btrfs_setxattr() are same, so the > diff obliterates the changes as described above. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style. > v4: born > fs/btrfs/acl.c | 7 +++++++ > fs/btrfs/props.c | 16 ++++++++++------ > fs/btrfs/xattr.c | 29 +++++++++-------------------- > fs/btrfs/xattr.h | 5 ++--- > 4 files changed, 28 insertions(+), 29 deletions(-) > > diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c > index 2b9c3394fc34..a8a1060c8cbe 100644 > --- a/fs/btrfs/acl.c > +++ b/fs/btrfs/acl.c > @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, > int ret, size = 0; > const char *name; > char *value = NULL; > + struct btrfs_root *root = BTRFS_I(inode)->root; > > switch (type) { > case ACL_TYPE_ACCESS: > @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, > goto out; > } > > + if (btrfs_root_readonly(root)) { > + ret = -EROFS; > + goto out; All the readonly checks needs to go first as it's the global condition, the following checks make sure that the arguments are valid and depend on the previous. > - ret = btrfs_setxattr(trans, inode, handler->xattr_name, > - NULL, 0, flags); > + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, > + flags); drive-by change > if (ret) > return ret; > > @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > ret = handler->validate(value, value_len); > if (ret) > return ret; > - ret = btrfs_setxattr(trans, inode, handler->xattr_name, > - value, value_len, flags); > + ret = btrfs_setxattr(trans, inode, handler->xattr_name, value, > + value_len, flags); same > if (ret) > return ret; > ret = handler->apply(inode, value, value_len); > if (ret) { > - btrfs_setxattr(trans, inode, handler->xattr_name, > - NULL, 0, flags); > + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, > + flags); And that one silently changes the return value semantics but looks like the other two, "just fixing the indentation". The original code does not set 'ret' as the whole operation returns what the property handler returned. The setxattr call here resets the property. If this is not the right, then fixed separately.
On 3/8/19 10:56 PM, David Sterba wrote: > On Fri, Mar 01, 2019 at 12:34:55PM +0800, Anand Jain wrote: >> Now btrfs_setxattr() is a very small function with just check for >> readonly FS and redirect the call to do_setxattr(). So instead >> move that checks to the parent functions and call do_setxattr() >> directly. Delete original btrfs_setxattr(), and rename do_setxattr() >> to btrfs_setxattr(). Also add few c-style. Kindly note the arguments >> of original do_setxattr() and original btrfs_setxattr() are same, so the >> diff obliterates the changes as described above. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style. >> v4: born >> fs/btrfs/acl.c | 7 +++++++ >> fs/btrfs/props.c | 16 ++++++++++------ >> fs/btrfs/xattr.c | 29 +++++++++-------------------- >> fs/btrfs/xattr.h | 5 ++--- >> 4 files changed, 28 insertions(+), 29 deletions(-) >> >> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c >> index 2b9c3394fc34..a8a1060c8cbe 100644 >> --- a/fs/btrfs/acl.c >> +++ b/fs/btrfs/acl.c >> @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, >> int ret, size = 0; >> const char *name; >> char *value = NULL; >> + struct btrfs_root *root = BTRFS_I(inode)->root; >> >> switch (type) { >> case ACL_TYPE_ACCESS: >> @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, >> goto out; >> } >> >> + if (btrfs_root_readonly(root)) { >> + ret = -EROFS; >> + goto out; > > All the readonly checks needs to go first as it's the global condition, > the following checks make sure that the arguments are valid and depend > on the previous. > >> - ret = btrfs_setxattr(trans, inode, handler->xattr_name, >> - NULL, 0, flags); >> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, >> + flags); > > drive-by change > >> if (ret) >> return ret; >> >> @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, >> ret = handler->validate(value, value_len); >> if (ret) >> return ret; >> - ret = btrfs_setxattr(trans, inode, handler->xattr_name, >> - value, value_len, flags); >> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, value, >> + value_len, flags); > > same > >> if (ret) >> return ret; >> ret = handler->apply(inode, value, value_len); >> if (ret) { >> - btrfs_setxattr(trans, inode, handler->xattr_name, >> - NULL, 0, flags); >> + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, >> + flags); > > And that one silently changes the return value semantics but looks like > the other two, "just fixing the indentation". The original code does not > set 'ret' as the whole operation returns what the property handler > returned. > > The setxattr call here resets the property. If this is not the > right, then fixed separately. That's copy and paste error. :-(. It should return ret of the handler->apply(). Because if the undo part which is btrfs_setxattr() is successful it means we fail silently. But at this place the handler->apply() can not fail. So the fail part is only theoretical. Also, theoretically in the original code the undo part is bit wrong, instead of resetting to the NULL it should reset back to the old value. I was trying to read the patch which is integrated if you have made any changes, so that I can send the fix. But I can't seems to find them. Thanks, Anand
diff obliterates the changes as described above. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v5: rename do_setxattr() to btrfs_setxattr(). change log update. fix c-style. v4: born fs/btrfs/acl.c | 7 +++++++ fs/btrfs/props.c | 16 ++++++++++------ fs/btrfs/xattr.c | 29 +++++++++-------------------- fs/btrfs/xattr.h | 5 ++--- 4 files changed, 28 insertions(+), 29 deletions(-) diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c index 2b9c3394fc34..a8a1060c8cbe 100644 --- a/fs/btrfs/acl.c +++ b/fs/btrfs/acl.c @@ -60,6 +60,7 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, int ret, size = 0; const char *name; char *value = NULL; + struct btrfs_root *root = BTRFS_I(inode)->root; switch (type) { case ACL_TYPE_ACCESS: @@ -95,7 +96,13 @@ static int do_set_acl(struct btrfs_trans_handle *trans, struct inode *inode, goto out; } + if (btrfs_root_readonly(root)) { + ret = -EROFS; + goto out; + } + ret = btrfs_setxattr(trans, inode, name, value, size, 0); + out: kfree(value); diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index 75a68d7d39a2..fa85a97fa13e 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -60,6 +60,7 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, const char *name, const char *value, size_t value_len, int flags) { + struct btrfs_root *root = BTRFS_I(inode)->root; const struct prop_handler *handler; int ret; @@ -70,9 +71,12 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, if (!handler) return -EINVAL; + if (btrfs_root_readonly(root)) + return -EROFS; + if (value_len == 0) { - ret = btrfs_setxattr(trans, inode, handler->xattr_name, - NULL, 0, flags); + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, + flags); if (ret) return ret; @@ -85,14 +89,14 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, ret = handler->validate(value, value_len); if (ret) return ret; - ret = btrfs_setxattr(trans, inode, handler->xattr_name, - value, value_len, flags); + ret = btrfs_setxattr(trans, inode, handler->xattr_name, value, + value_len, flags); if (ret) return ret; ret = handler->apply(inode, value, value_len); if (ret) { - btrfs_setxattr(trans, inode, handler->xattr_name, - NULL, 0, flags); + ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0, + flags); return ret; } diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c index 2cbde7cac14b..358e6aed01ca 100644 --- a/fs/btrfs/xattr.c +++ b/fs/btrfs/xattr.c @@ -76,9 +76,11 @@ int btrfs_getxattr(struct inode *inode, const char *name, return ret; } -static int do_setxattr(struct btrfs_trans_handle *trans, - struct inode *inode, const char *name, - const void *value, size_t size, int flags) +/* + * @value: "" makes the attribute to empty, NULL removes it + */ +int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, + const char *name, const void *value, size_t size, int flags) { struct btrfs_dir_item *di = NULL; struct btrfs_root *root = BTRFS_I(inode)->root; @@ -217,22 +219,6 @@ static int do_setxattr(struct btrfs_trans_handle *trans, return ret; } -/* - * @value: "" makes the attribute to empty, NULL removes it - */ -int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, - const char *name, const void *value, size_t size, int flags) -{ - struct btrfs_root *root = BTRFS_I(inode)->root; - - ASSERT(trans); - - if (btrfs_root_readonly(root)) - return -EROFS; - - return do_setxattr(trans, inode, name, value, size, flags); -} - ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size) { struct btrfs_key key; @@ -357,6 +343,9 @@ static int btrfs_xattr_handler_set(const struct xattr_handler *handler, name = xattr_full_name(handler, name); + if (btrfs_root_readonly(root)) + return -EROFS; + trans = btrfs_start_transaction(root, 2); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -445,7 +434,7 @@ static int btrfs_initxattrs(struct inode *inode, strcpy(name, XATTR_SECURITY_PREFIX); strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name); err = btrfs_setxattr(trans, inode, name, xattr->value, - xattr->value_len, 0); + xattr->value_len, 0); kfree(name); if (err < 0) break; diff --git a/fs/btrfs/xattr.h b/fs/btrfs/xattr.h index 471fcac6ff55..26ff6c11774f 100644 --- a/fs/btrfs/xattr.h +++ b/fs/btrfs/xattr.h @@ -12,9 +12,8 @@ int btrfs_getxattr(struct inode *inode, const char *name, void *buffer, size_t size); -int btrfs_setxattr(struct btrfs_trans_handle *trans, - struct inode *inode, const char *name, - const void *value, size_t size, int flags); +int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, + const char *name, const void *value, size_t size, int flags); ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size); int btrfs_xattr_security_init(struct btrfs_trans_handle *trans,