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

[Anand Avati]

These converters are used in vfs_glusterfs for storing posix acls
as xattrs (where acl_set_file does not work)

Signed-off-by: Anand Avati <av...@redhat.com>
---
source3/modules/vfs_posixacl.c | 7 ++-----
source3/modules/vfs_posixacl.h | 3 +++
2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/source3/modules/vfs_posixacl.c b/source3/modules/vfs_posixacl.c
index 6963aae..7733832 100644
--- a/source3/modules/vfs_posixacl.c
+++ b/source3/modules/vfs_posixacl.c
@@ -26,10 +26,7 @@

static bool smb_ace_to_internal(acl_entry_t posix_ace,
struct smb_acl_entry *ace);
-static struct smb_acl_t *smb_acl_to_internal(acl_t acl, TALLOC_CTX *mem_ctx);
static int smb_acl_set_mode(acl_entry_t entry, SMB_ACL_PERM_T perm);
-static acl_t smb_acl_to_posix(const struct smb_acl_t *acl);
-

/* public functions - the api */

@@ -213,7 +210,7 @@ static bool smb_ace_to_internal(acl_entry_t posix_ace,
return True;
}

-static struct smb_acl_t *smb_acl_to_internal(acl_t acl, TALLOC_CTX *mem_ctx)
+struct smb_acl_t *smb_acl_to_internal(acl_t acl, TALLOC_CTX *mem_ctx)
{
struct smb_acl_t *result = sys_acl_init(mem_ctx);
int entry_id = ACL_FIRST_ENTRY;
@@ -270,7 +267,7 @@ static int smb_acl_set_mode(acl_entry_t entry, SMB_ACL_PERM_T perm)
return acl_set_permset(entry, permset);
}

-static acl_t smb_acl_to_posix(const struct smb_acl_t *acl)
+acl_t smb_acl_to_posix(const struct smb_acl_t *acl)
{
acl_t result;
int i;
diff --git a/source3/modules/vfs_posixacl.h b/source3/modules/vfs_posixacl.h
index b0fe841..424cfe9 100644
--- a/source3/modules/vfs_posixacl.h
+++ b/source3/modules/vfs_posixacl.h
@@ -44,5 +44,8 @@ int posixacl_sys_acl_delete_def_file(vfs_handle_struct *handle,

NTSTATUS vfs_posixacl_init(void);

+struct smb_acl_t *smb_acl_to_internal(acl_t acl, TALLOC_CTX *mem_ctx);
+acl_t smb_acl_to_posix(const struct smb_acl_t *acl);
+
#endif

--
1.7.1

[Simo]

Reading the module code, the reason you are making these calls public is
to call them after you use acl_copy_int on the acl buffer.

If I understand it correctly that public function the buffer is an
exportable representation of an API.
So maybe, instead of making these internal functions public we can add a
new interface to the VFS that returns this public exportable buffer
instead and let the API caller call and internal function that does
acl_copy_int + smb_acl_to_internal , and the reverse ?

Not sure if this is too Linux specific though.

Simo.


--
Simo Sorce
Samba Team Member <si...@samba.org>
Principal Software Engineer at Red Hat, Inc. <si...@redhat.com>

[Anand Avati]

--
1.7.1

[Andrew Bartlett]

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
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org

[Simo]

On 05/24/2013 04: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.
>

I proposed to move acl handling in the core at least for 4.1, see my
review email.

Simo.

[Anand Avati]

> 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.

Avati

[Andrew Bartlett]

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?

[Anand Avati]

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

[Jeremy Allison]

On Fri, May 24, 2013 at 04:21:16PM -0700, Anand Avati wrote:
>
> 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.

Yes, that's the only safe way to do it at present I think.

Cheers,

Jeremy.