> +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.
> +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?
> +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?
> +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);
> + 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?
|