Re: [PATCH v18 21/22] ext4: Add richacl support

On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig [off-list ref] wrote:

quoted

+static int
+__ext4_set_richacl(handle_t *handle, struct inode *inode, struct richacl *acl)
+{
+     const int name_index = EXT4_XATTR_INDEX_RICHACL;
+     umode_t mode = inode->i_mode;
+     int retval, size;
+     void *value;
+
+     if (richacl_equiv_mode(acl, &mode) == 0) {
+             inode->i_ctime = ext4_current_time(inode);
+             inode->i_mode = mode;
+             ext4_mark_inode_dirty(handle, inode);
+             return __ext4_remove_richacl(handle, inode);
+     }

Should this check for a NULL acl instead of special casing that
in ext4_set_richacl?

I'm not sure I understand what you mean. When iop->set_richacl is
called with a richacl that is mode-equivalent, the file permission
bits need to be updated and any existing acl needs to be removed.
Doing this at the vfs level would result in two calls, iop->setattr
and iop->set_richacl, which can cause problems. To remove an existing
acl without setting the mode, set_richacl is called with a NULL
richacl.

__ext4_set_richacl() was split into __ext4_set_richacl() and
__ext4_remove_richacl() to align with the xfs code due to the
following comment from Dave Chinner:
  http://oss.sgi.com/archives/xfs/2015-10/msg00354.html

Diff here:
  https://git.kernel.org/cgit/linux/kernel/git/agruen/linux-richacl.git/diff/fs/ext4/richacl.c?id=richacl-2015-10-16&id2=richacl-2015-10-12

quoted

+int
+ext4_init_richacl(handle_t *handle, struct inode *inode, struct inode *dir)
+{
+     struct richacl *acl = richacl_create(&inode->i_mode, dir);
+     int error;
+
+     error = PTR_ERR(acl);
+     if (IS_ERR(acl))
+             return error;

        if (IS_ERR(acl))
                return PTR_ERR(acl);

quoted

+     if (acl) {
+             error = __ext4_set_richacl(handle, inode, acl);
+             richacl_put(acl);
+     }

Shouldn't richacl_create return NULL if the ACL is equivalent to the
mode bits instead of letting every filesystem figure that out on it's
own?

Hm, that's what it does?

Thanks,
Andreas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs