Patch review -- file_id_string_tos()?

[Andrew Bartlett]

On Tue, 2012-10-30 at 12:24 +0100, Michael Adam wrote:
> On 2012-10-30 at 19:24 +1100, Andrew Bartlett wrote:
> > On Tue, 2012-10-30 at 08:34 +0100, Volker Lendecke wrote:
> > > On Tue, Oct 30, 2012 at 07:52:45AM +1100, Andrew Bartlett wrote:
> > > > On Mon, 2012-10-29 at 16:47 +0100, Volker Lendecke wrote:
> > > > > Hi, Andrew!
> > > > > 
> > > > > The attached patch removes some code duplication. I send
> > > > > this to you because you introduced file_id_string() without
> > > > > _tos() recently.
> > > > > 
> > > > > Please review and push if it seems appropriate to you.
> > > > 
> > > > I'll be very glad to remove another implicit return on talloc_tos().
> 
> I am actually not certain that I completely understand what you
> want to express here.  

Neither am I.  I misread the patch, and was a little confused as to what
I had to do with it.  

> Do you want to say that you don't want to
> review or ack Volker's patch which is an obvious and simple
> improvement to your previous patch beacause you had further
> structural improvements in mind?

I would very much like to see the implicit return of memory on
talloc_tos() removed.  I think I misread the patch as actually removing
one of those. 

> While it is certainly a valid point of view to not want implicit
> returns on talloc_tos(), this should not be a reason to not
> review/ack/push such obvious improvements of existing code. You
> never know how long it will take until you find the time to do your
> improvements that might render this current patch unnecessary...
> I.e. we should improve the current code, even if the improvement
> should be superseeded within a couple of weeks, which can easily
> turn to months. :-)
> 
> > > Actually, this one is the one (and only one) place where I
> > > would argue returning a string on talloc_tos() is a good
> > > thing. In fact, talloc_tos() was invented for this case:
> > > DEBUG. I did not find an easy way to print SIDs in a sane
> > > way without much more effort on the caller. Tridge had
> > > invented sid_string_dbg(), but this crashes when another
> > > function call called from the DEBUG args also calls
> > > sid_string_dbg().
> > > 
> > > I did think a LOT about the DEBUG problem. If you can come
> > > up with a solution that does not involve a lot of explicit
> > > talloc_tos() args, I'd be happy to look at it.
> > 
> > I would prefer explicit talloc_tos() args.
> 
> Generally speaking, you are certainly right here!
> 
> The special case Volker has detailed, might be a valid exception
> of the rule though, so as not to make it extremely difficult to
> use such dynamic strings in DEBUG messages.

Is it much more painful to pass talloc_tos() as the argument rather than
call the _tos() functions?

Andrew Bartlett

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