[PATCH] vfs_acl_xattr: avoid setting POSIX acls if "ignore system acls" is set

[Hemanth Thummala]

Hi,

I have a question.

If the intention of “ignore filesystem ACLs” is to just ignore the underlying POSIX ACLs, we shouldn’t be ignoring the NTACL hash computation in get_nt_acl_internal() right? Otherwise we would end up not using HASH(for NTACL) at all. As Uri pointed out, we might even go back back to version 1 which has just SD. Is intentional to skip the hash check for NTACL when “ignore filesystem ACLs” is set? If so, is it for performance reasons?

Also I found a comment which is misleading.


diff --git a/source3/modules/vfs_acl_common.c b/source3/modules/vfs_acl_common.c
index f73e80c..ad5c8f8 100644
--- a/source3/modules/vfs_acl_common.c
+++ b/source3/modules/vfs_acl_common.c
@@ -477,8 +477,6 @@ static NTSTATUS get_nt_acl_internal(vfs_handle_struct *handle,
                                                                 &sys_acl_blob);
                }
 
-               /* If we fail to get the ACL blob (for some reason) then this
-                * is not fatal, we just work based on the NT ACL only */
                if (ret == 0) {
                        status = hash_blob_sha256(sys_acl_blob, sys_acl_hash_tmp);
                        if (!NT_STATUS_IS_OK(status)) {


We should either remove this completely or add as part of else(error condition) case. I think we can remove this comment as we have already have comment on fall through part.

Thanks,

Hemanth.

On 3/22/16, 12:31 PM, "samba-technical on behalf of Uri Simchoni" <samba-technical-bounces at lists.samba.org on behalf of uri at samba.org> wrote:

>On 03/22/2016 09:19 PM, Richard Sharpe wrote:
>> On Tue, Mar 22, 2016 at 12:03 PM, Uri Simchoni <uri at samba.org> wrote:
>>> On 03/22/2016 05:14 PM, Richard Sharpe wrote:
>>>>
>>>>
>>>> Can you also remove that stupid time value that was added? It screws
>>>> up ACL deduplication if you have a file system that can do that.
>>>>
>>>> At least it should be configurable.
>>>>
>>>  From brief look into the code I can't understand the purpose of the time
>>> stamp (only idea that comes to mind is for extra-debugging info). I'll look
>>> through the history but if someone can tell me please do so.
>>>
>>> Same goes for "description" which seems (again, brief look) to be a place
>>> holder for something.
>>>
>>>> Actually, it should also be possible to configure V3 vs V4 formats as
>>>> well.
>>>>
>>>
>>> Cannot see the use case for that -
>>> v4 seems to be more efficient for write-once-read-many which is the common
>>> case. If there's a file system in which fetching the NT ACL is faster than
>>> fetching the POSIX ACL (and hence v3 would be faster for read), then I
>>> suppose it is not a POSIX-like file system and should not be using
>>> vfs_acl_xattr in the first place.
>>
>> I am not sure I understand this. We are using vfs_acl_xattr to store
>> SDs. That it also does stuff with Posix ACLs is unwelcome from our
>> point of view, however, we can live with the unused extra 64-bytes of
>> has info if the time field goes away and with your extension of ignore
>> system acls is adopted.
>>
>What I meant was:
>1. if my extension is adopted, and ignore-system-acls is used - only v3, 
>no time stamps, no description, no posix ACLs, Windows-only use-case 
>happy, dedup happy.
>2. What I wrote about v3 vs v4 and the need for "configurable" therefore 
>relates only to the case where system ACLs are NOT ignored, and what's 
>the best algorithm to sync the two - seems to me like v4 is better 
>(except for the time stamp which I don't understand) and therefore 
>should always be used - can't see why it should be configurable.
>
>One other question that may come to mind is - why stop there and not use 
>v1 instead, if system ACLs are ignored, that is why waste 64 bytes. The 
>original reason I did it that way was some (mis-?)perception that v1 and 
>v2 are "legacy" or obsolete. But even after second examination, I think 
>using v3 is a way to reset the NT ACL if one later chooses to not-ignore 
>system ACLs.
>
>Hope that clears things up,
>Uri.
>
>