why do we call ensure_canon_entry_valid on get?

[Andrew Bartlett]

On Fri, 2012-10-05 at 14:36 -0700, Jeremy Allison wrote:
> On Sat, Oct 06, 2012 at 07:27:28AM +1000, Andrew Bartlett wrote:
> > On Fri, 2012-10-05 at 08:44 -0700, Jeremy Allison wrote:
> > > On Fri, Oct 05, 2012 at 06:01:08PM +1000, Andrew Bartlett wrote:
> > > > On Tue, 2012-10-02 at 22:28 +0200, Jeremy Allison wrote:
> > > > > commit 6575d1d34fee45c7a965c7c9641cc52b566a9e7f
> > > > > Author: Jeremy Allison <jra at samba.org>
> > > > > Date:   Tue Oct 2 10:15:54 2012 -0700
> > > > > 
> > > > >     When setting a non-default ACL, don't forget to apply masks to
> > > > > SMB_ACL_USER and SMB_ACL_GROUP entries.
> > > > 
> > > > Jeremy,
> > > > 
> > > > With this change, does this mean we have changed the mapping between
> > > > posix ACLs and NT ACLs?
> > > > 
> > > > If so, I'm concerned that any NT ACLs that have been set with
> > > > vfs_acl_xattr will be invalidated, as the hash won't match up.  
> > > 
> > > Andrew, it *never* matters on set, it only matters on get.
> > > 
> > > On set we will a re-hash, so changing the mapping on sets
> > > doesn't matter.
> > 
> > Sure, but this code seems to be in the get codepath,
> > ensure_canon_entry_valid() is called via canonicalise_acl() from
> > posix_get_nt_acl_common(). 
> 
> static bool ensure_canon_entry_valid(connection_struct *conn,
>                                         canon_ace **pp_ace,
>                                         bool is_default_acl,
>                                         const struct share_params *params,
>                                         const bool is_directory,
>                                         const struct dom_sid *pfile_owner_sid,
>                                         const struct dom_sid *pfile_grp_sid,
>                                         const SMB_STRUCT_STAT *pst,
>                                         bool setting_acl)
> 
> Note the last bool parameter, which is only true when we're
> setting an ACL.

That resolves my confusion, and my concern about your patch.

It leaves me with puzzlement as to what the routine is used for in the
get codepath. 

(man acl):
     The acl_valid() function checks the ACL referred to by the argument acl for validity.

     The three required entries ACL_USER_OBJ, ACL_GROUP_OBJ, and ACL_OTHER must exist exactly once in the ACL. If the ACL
     contains any ACL_USER or ACL_GROUP entries, then an ACL_MASK entry is also required. The ACL may contain at most one
     ACL_MASK entry.

Are there systems where this doesn't hold?  Is this for the case where
we don't have an ACL at all (just file modes)?  (If we could more
clearly separate the get and set code-paths this would be much less
confusing).

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org