On Fri, Mar 11, 2016 at 3:27 PM, Christoph Hellwig [off-list ref] wrote:
quoted
+static inline int
+ext4_acl_chmod(struct inode *inode, umode_t mode)
+{
+ if (IS_RICHACL(inode))
+ return richacl_chmod(inode, inode->i_mode);
+ return posix_acl_chmod(inode, inode->i_mode);
+}
Thi isn't ext4-specific and potentially duplicated in every caller.
Please provide this as a common helper.
Also while we're at it, the mode argument is ignore and the function
always uses inode->i_mode instead.
quoted
+ext4_get_richacl(struct inode *inode)
+{
+ const int name_index = EXT4_XATTR_INDEX_RICHACL;
+ void *value = NULL;
+ struct richacl *acl = NULL;
+ int retval;
+
+ retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
+ if (retval > 0) {
+ value = kmalloc(retval, GFP_NOFS);
+ if (!value)
+ return ERR_PTR(-ENOMEM);
+ retval = ext4_xattr_get(inode, name_index, "", value, retval);
+ }
+ if (retval > 0) {
+ acl = richacl_from_xattr(&init_user_ns, value, retval);
+ if (acl == ERR_PTR(-EINVAL))
+ acl = ERR_PTR(-EIO);
Shouldn't richacl_from_xattr return the error pointer that ->get_richacl
callers expect?
The xattr representation is the same on disk and at the xattr syscall
layer, and so richacl_from_xattr is used for converting into the
in-memory representation in both cases. The error codes are not the
same when a user supplies an invalid value via setxattr or NFS and
when an invalid xattr is read from disk though. I'll add a parameter
to richacl_from_xattr to make this more explicit.
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 the
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?