From: Andreas Gruenbacher Subject: Re: [PATCH v18 21/22] ext4: Add richacl support Date: Wed, 16 Mar 2016 23:38:32 +0100 Message-ID: References: <1456733847-17982-1-git-send-email-agruenba@redhat.com> <1456733847-17982-22-git-send-email-agruenba@redhat.com> <20160311142719.GG14808@infradead.org> <20160315071707.GF19747@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: Alexander Viro , "Aneesh Kumar K.V" , "J. Bruce Fields" , Linux NFS Mailing List , "Theodore Ts'o" , linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux API , Trond Myklebust , LKML , XFS Developers , Andreas Dilger , linux-fsdevel , Jeff Layton , linux-ext4 , Anna Schumaker To: Christoph Hellwig Return-path: In-Reply-To: <20160315071707.GF19747-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-ext4.vger.kernel.org On Tue, Mar 15, 2016 at 8:17 AM, Christoph Hellwig wrote: > On Mon, Mar 14, 2016 at 12:08:31AM +0100, Andreas Gruenbacher wrote: >> 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. > > Better add a wrapper instead of a parameter. > >> >> >> +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 > > ext4_set_richacl checks for a NULL acl pointer and then calls into > __ext4_remove_richacl. I'd rather have that special casing in one > place. Those are two different cases: the first is where ext4_set_richacl is called with a NULL acl to remove an existing ACL; the second is where ext4_set_richacl is called with a mode-equivalent ACL to set the mode and remove any existing ACL. The check for mode-equivalent ACLs is in __ext4_set_richacl and not in ext4_set_richacl because an inherited ACL (ext4_init_acl) can also be mode-equivalent. Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html