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

[Jeremy Allison]

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

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

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

Jeremy.