[PATCH] Rework S3 DEBUG system, impact on libsmbclient

[Andrew Bartlett]

On Mon, 2010-11-01 at 21:16 -0400, Derrell Lipman wrote:
> On Mon, Nov 1, 2010 at 20:18, Andrew Bartlett <abartlet at samba.org> wrote:
> > Derrell,
> >
> > I've reworked the Samba3 debug system, to make it more self-contained
> > (no longer exposing global static variables, for example) and to ease a
> > merge between the s3 and s4 implementations.  This included a change in
> > the handling of smbc_set_option(ctx, "debug_to_stderr", 1) and
> > smbcSetDebugToStderr(ctx, 1).
> ...
> > The code asks that we might get a per-thread debug 'dbf' at some point,
> > but this hasn't happened since, so I think this is the most appropriate
> > way to deal with this for the time-being.  (and per-thread wouldn't
> > particular help with multiple contexts anyway)
> >
> > Is this an OK change in behaviour?
> 
> Hi Andrew,
> 
> I would find it pretty difficult to believe that anyone would have
> debug to stderr in one context, and to stdout in another, but since
> this is a client library, we'd really have no way to know. I suppose
> there could be some circumstances where that could prove useful, but I
> don't think we must retain that capability. If this change is going
> into a major release, I see no problem with this concept at all. If
> it's going into a minor release, I'd have a bit more reservation.

This is only proposed for master (and therefore eventually 4.0).  I'm
not proposing to merge this to 3.6, but even then it would still be a
major release.

> All of the code changes look very reasonable. I have a little bit of
> concern with smbc_getOptionDebugToStderr() calling
> debug_get_output_is_stderr() directly, because that could potentially
> get out of sync with context->internal->debug_stderr. Since a change
> from stdout -> stderr is now permanent per your "ratchet", it might
> make sense to remove context->internal->debug_stderr all together,
> which would remove the sync issue. That would involve only a few
> changes, I think:
> 
> 1. In libsmb_getset.c, change smbc_getOptionDebugToStderr() to return
> debug_get_output_is_stderr(). You've already done this in one of your
> patches.
> 
> 2. Also in libsmb_getset.c, change smbc_setOptionDebugToStderr() to
> directly invoke setup_logging("libsmbclient", DEBUG_STDERR), or maybe
> test whether it's already set and do it only if not already done.
> (Depends on your "ratchet" mechanism and whether it throws an error if
> one tries to set it twice.)

That's a good idea.  

> 3. In libsmb_context.c, delete the block of code that evaluates
> context->internal->debug_stderr. There's no reason to do that per
> context any longer. The default of using stdout is set in
> SMBC_module_init() which is called upon the very first context
> allocation, and that can remain.
> 
> 4. Finally, remove the debug_stderr member from the SMBC_internal_data
> structure in include/libsmb_internal.h
> 
> I'd recommend specifying to the linker that the symbols setup_logging
> and debug_get_output_is_stderr should not be accessible in the
> libsmbclient.{a,so} so that the user must use
> smbc_[gs]etOptionDebugToStderr() to set and retrieve the output
> channel.

Except for the linker details, the rest of this sounds very reasonable.
I'll make enquiries with others who understand the linker behaviour
better about the symbol hiding, and do this if we do it for any other
symbol. 

Thanks for looking over this!

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20101102/e48836fd/attachment.pgp>