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

Andrew Bartlett abartlet at samba.org
Wed Oct 10 18:27:56 MDT 2012


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

Thanks.  

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




More information about the samba-technical mailing list