[PATCH 2/3 v4] vfs_posixacl: expose acl_t <--> smb_acl_t converter functions

[Anand Avati]

On 5/24/13 2:45 PM, Andrew Bartlett wrote:
> On Fri, 2013-05-24 at 10:41 -0700, Anand Avati wrote:
>> On 5/24/13 1:41 AM, Andrew Bartlett wrote:
>>> On Thu, 2013-05-23 at 22:29 -0400, Anand Avati wrote:
>>>> These converters are used in vfs_glusterfs for storing posix acls
>>>> as xattrs (where acl_set_file does not work)
>>>
>>> I was all keen to submit this to autobuild, but it doesn't even compile:
>>>
>>> [2693/3956] Compiling source3/lib/sysacls.c
>>> In file included from ../source3/lib/sysacls.c:26:0:
>>> ../source3/modules/vfs_posixacl.h:47:39: error: unknown type name
>>> ‘acl_t’
>>> ../source3/modules/vfs_posixacl.h:48:1: error: unknown type name ‘acl_t’
>>> Waf: Leaving directory `/data/samba/git/samba-push/bin'
>>> Build failed:  -> task failed (err #1):
>>> 	{task: cc sysacls.c -> sysacls_92.o}
>>> make: *** [all] Error 1
>>>
>>> I think we need to make glusterfs depend on posix ACLs (presumably the
>>> converter function you use) and have these header definitions depend on
>>> whatever provides acl_t.
>>>
>>> In general we don't like conditional stuff in our headers, which is why
>>> there is the whole smb_acl stuff in the first place, but this is a bit
>>> of a special case.
>>>
>>> Andrew Bartlett
>>>
>>
>>
>> Looks like a slip between my v3 and v4 patches again. In v3 I had
>> '#include <sys/acl.h>' in vfs_posixacl.h in the 3rd patch (vfs_glusterfs
>> patch). I was trying to move that line into the 2nd patch (which changes
>> vfs_posixacl.h) and somehow it has gotten missed! Will resubmit.
>
> OK, so that's probably because you tried to only send in the 3rd path
> last time (at least, I didn't see a new version of the others, so it
> could be my mistake).
>
> Looking again at the code, I'm now not at all convinced that exposing
> these functions from this file is the right thing to do.  This is a vfs
> module, you shouldn't have one vfs module depend on another.  This is
> only appearing to work because of what is currently set as a 'static'
> module.
>
> Nor should we be adding <sys/acl.h> directly to a header - we always go
> via our system/filesys.h header, to ensure portability.  The same
> applies to the glusterfs mdoule btw.
>
> That means that these helper functions need to move to a different
> helper library, not just be exposed as-is.  Then that file has to be
> built only if we have acl_t and and acl iterator functions (essentially
> HAVE_POSIX_ACLS)
>
> Finally, this does make me concerned:  How is the link between the OS
> storage format for ACLs (exposed via acl_copy_int et al) and what
> glusterfs uses defined?  That is, can glusterfs be used cross-platform,
> in a situation where each host OS might have a different 'on-disc' ACL
> format, or will glusterfs always be linux-only?
>
> Andrew Bartlett
>

Please see my comment in the 3/3 patch in reply to Simo's comments. 
Gluster's ACL format is byte compatible with Linux's ACL byte format. 
Therefore acl_copy_int() works. Your concern is true on non-Linux 
systems (though I'm not sure if they really use a different format, but 
a bad assumption in any case). So I propose (as mentioned in that mail) 
to have a smb_acl_t to gluster-acl native converter function which will 
look similar to the one in vfs_posixacl.c, but will decouple.

Feedback appreciated!
Avati