Patch review -- file_id_string_tos()?

[Michael Adam]

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.  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?

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.

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121030/c396c4af/attachment.pgp>