returning values on talloc_tos() (was Re: [PATCH][WIP] Make vfs_acl_xattr use hash of the posix ACL)

simo idra at samba.org
Wed Oct 10 18:39:48 MDT 2012


On Thu, 2012-10-11 at 11:27 +1100, Andrew Bartlett wrote:
> On Wed, 2012-10-10 at 16:31 -0700, Jeremy Allison wrote:
> > On Wed, Oct 10, 2012 at 07:37:56PM +1100, Andrew Bartlett wrote:
> > > On Wed, 2012-10-10 at 08:37 +0200, Volker Lendecke wrote:
> > > > On Tue, Oct 09, 2012 at 02:41:04PM -0700, Jeremy Allison wrote:
> > > > > On Wed, Oct 10, 2012 at 07:50:03AM +1100, Andrew Bartlett wrote:
> > > > > Right now we only have 3 VFS calls that take an explicit
> > > > > context. I'm not sure adding context to the ACL calls
> > > > > is the right move here. It's doable, but I'd like more
> > > > > input before we decide.
> > > > > 
> > > > > Richard, Volker or Metze, can you comment ? You've
> > > > > guys have probably written more VFS modules than anyone
> > > > > else.
> > > > > 
> > > > > Explicit context for [F]GET_NT_ACL() calls or talloc_tos() ?
> > > > 
> > > > Explicit context please.
> > > 
> > > Thanks Volker.  I've prepared the attached two patches to do this.
> > > 
> > > I'm testing these and a broken-up set of patches for my other changes
> > > now.  For the curious, the whole series is at
> > > https://git.samba.org/abartlet/samba.git/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-acls2
> > 
> > Thanks Andrew, that looks much better !
> > 
> > Only a couple of (minor) nits.
> > 
> > 1). In this patch :
> > 
> > smbd: Add mem_ctx to {f,}get_nt_acl VFS call
> > 
> > You state : "As the ACL transformation allocates and then no longer needs a great
> > deal of memory, a talloc_stackframe() call is used to contain the
> > memory that is not returned further up the stack."
> > 
> > There are extra talloc_stackframes() used in other patches as well.
> > 
> > Is this ("a great deal of memory") true ? It's normally at max a few 10's
> > of kilobytes I think. Is this a case of premature optimization ? I prefer
> > to reduce extraneous talloc_stackframes() if possible, as they may break
> > assumptions the rest of the code is making. I don't see why talloc_tos()
> > can't be used here instead in most of the cases you're using talloc_stackframe().
> 
> Given that we memory we specifically return is allocated on the supplied
> context, what assumptions are you thinking of?
> 
> No other memory is returned, and this makes it very clear exactly what
> the expected lifetimes are.  
> 
> > 2). Use of talloc_steal(). I know what it does, I just think
> > using talloc_move() is much cleaner and reduces possibility
> > of error by re-using a stolen pointer :-).
> > 
> > I much prefer:
> > 
> > 	a = talloc_move(new_ctx, &a);
> > 
> > to:
> > 
> > 	talloc_steam(new_ctx, a);
> > 
> > But I understand this can be a matter of personal preference :-).
> > But I *really* hate :
> > 
> > 	acl_wrapper.access_acl
> > 			= talloc_steal(frame,
> > 					smb_vfs_call_sys_acl_get_file(handle,
> > 								path_p,
> > 								SMB_ACL_TYPE_ACCESS, 
> > 								frame));
> 
> I'll find and fix these two (already removed another one of those) - the
> memory context is specified - as frame, the last parameter - it doesn't
> need the talloc_steal!
> 
> > That's just asking for trouble (IMHO) :-). Yes I know it's safe
> > against NULL pointer return, but it's just being too clever for
> > our own good (IMHO).
> > 
> > Neither issue are show-stoppers for me though :-).
> 
> As I think I've addressed all the issues raised by you and Simo, I
> propose to autobuild everything except the vfs_acl_common changes soon.
> (I still need to add fix and add tests for actual use of the new hash
> code).
> 
> https://git.samba.org/abartlet/samba.git/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/fix-acls2

Any chance you can keep 80 cols limits in the reindenting patch ?
It would be nice to fix that too while there.

Simo.


-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>



More information about the samba-technical mailing list