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

Jeremy Allison jra at
Wed Oct 10 17:31:43 MDT 2012

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

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

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);


	talloc_steam(new_ctx, a);

But I understand this can be a matter of personal preference :-).
But I *really* hate :

			= talloc_steal(frame,

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


More information about the samba-technical mailing list