Handle IDMAP_BOTH in posix_acls.c

[Christian Ambach]

On 05/04/2012 04:17 AM, Andrew Bartlett wrote:

> I've looked at this, and I have three concerns:
>   - How is this code tested
>   - What is the purpose of the sidmap?

I think the sidmap was initially added to give users the possibility to 
define that a certain SID should better be replaced by another one when 
compiling the ACL. I am not aware of anyone actually using the function 
and I have troubles remembering why this functionality was added five 
years ago. I have crawled my mail archives without success.

sidmap was added to the code in the process of syncing Tridge's 
samba-ctdb bzr tree to Samba, so the history is lost as the bzr repo is 
not accessible any more. Maybe someone still has a copy around?

Maybe it was meant to be able to deal with SIDs which cannot be mapped 
for whatever reason (e.g. somebody copies file from its local machine to 
the share and the ACLs contain SIDs of the local machine).

>   - What is the purpose of the lookup_sid() call?
I would guess this relates to the sidmapping functionality above: if the 
SID cannot be found, then check if there is a replacement to use defined 
in the sidmap database.

> On the last point, it seems we have a potentially quite inefficient
> lookup_sid call in nfs4_acls.c:smbacl4_fill_ace4().  This is called for
> every ACE on every ACL set.
>
> I'm hoping this was just added out of paranoia, but it seems to be tied
> in to the sidmap.  In any case the posix ACL code doesn't do this, and
> it would work best if we remove it to support IDMAP_BOTH (but, due to
> doing this lookup, it also means that idmap both will simply be ignored
> for nfs4 in the interim).

I would vote to remove the whole sidmap code. I do not think this has 
seen real use yet and usecases for it seem to be very rare.
It's also not documented in README.nfs4acls.txt

Cheers,
Christian