Thanks, Haohui. In that case, let's work towards getting some variant of the v4 patch committed. I'll get to work on a v5. Some of the simplifications you proposed might not be feasible due to a few complexities in the requirements. See details below. Bottom line: if I can get the unit tests passing with these simplifications, then we can keep them.
There are a couple of additional complexities to the logic: 1) This promotion must only be triggered if the ACL spec has specified some kind of change to the default ACL. A change to only the access ACL must not trigger promotion. 2) Promotion only occurs for the base entries that are missing from the default ACL. It must not overwrite a default entry that the user provided in the ACL spec. This is why the logic is tracking both access entries and default entries. Example: if user specifies default owner entry and default group entry, but no default other entry, then the logic must use the provided default owner and default group entries and then copy the access ACL owner entry to the default ACL. 3) The access mask is an access entry with no name, but the logic must never promote it to a default mask.
A few of the challenges with this logic: 1) Automatic mask calculation must not trigger for the default ACL if the ACL spec is changing only the access ACL (and vice versa). 2) The logic must know whether a mask entry is coming from the existing ACL or being added by the ACL spec. The version in the ACL spec must override the automatic calculation. 3) In the case of filterAclEntriesByAclSpec, the logic must have knowledge that some kind of change was made to either the access ACL or default ACL. The list of ACL entries alone isn't sufficient to know that, because absence of something in that list does not necessarily imply deletion. All of these requirements drove the state tracking related to scopeDirty/maskDirty/providedMask.
I also propose to move the compareTo() function AclEntry out to a new comparator inside AclTransformationLogic.
Yes, we can do that. Just so I understand the motivation, is this to reduce the footprint of code shipped client side, particularly code that is sensitive to server side implementation details? (If so, then I agree with you.)
Is it possible to have multiple entries whose type equals to ACCESS, and empty names? If the answer is yes, copyDefaultsIfNeeded will always pick up the last entry. Is it expected?
I didn't quite follow this question, because ACCESS is a scope, and the types are USER, GROUP, MASK and OTHER. Within any ACL, the combination of scope + type + name is unique for each entry. Attempting to create entries with duplicate scope + type + name in the same ACL would get rejected during the validation checks for duplicate entries. Therefore, for any invocation of copyDefaultsIfNeeded, I'd expect each of those code paths to execute at most once.
How are you going to use these functions?
Each FSNamesystem method that modifies an ACL will get the current ACL from the inode feature, pass the existing ACL + the ACL spec to the corresponding AclTransformation method, and then set the resulting new ACL back on the inode feature. FSNamesystem#removeAcl is unique in that it can simply remove the ACL feature. There will be some integration with the existing FSNamesystem#setPermission too. If calling setPermission on an inode with an ACL, then it will convert the client-supplied FsPermission to a minimal ACL (containing exactly the 3 base entries), and then call AclTransformation#mergeAclEntries to combine that minimal ACL with the existing ACL. Additionally, FSNamesystem#removeAclEntries, FSNamesystem#removeDefaultAcl, and FSNamesystem#setAcl might result in reducing back to a minimal ACL that can be represented as a 16-bit FsPermission. We'll detect these scenarios and handle it by removing the ACL feature from the inode. That's a summary of everything I can think of right now related to how these functions will be used.