[PATCH] Rework S3 DEBUG system, impact on libsmbclient

[Derrell Lipman]

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.

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

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.

Cheers,

Derrell