[PATCHES] Logging to multiple debug backends

Christof Schmitt cs at samba.org
Mon Mar 16 15:07:31 MDT 2015


On Sat, Mar 14, 2015 at 02:52:05PM +1100, Martin Schwenke wrote:
> On Fri, 13 Mar 2015 11:57:13 -0700, Christof Schmitt <cs at samba.org>
> wrote:
> 
> > The attached patches add support for multiple debug backends. Today's
> > code only has the notion of logging to a file, and optionally logging to
> > syslog through the 'syslog' and 'syslog only' parameters.  With the
> > patches applied, this becomes more flexible. Multiple logging backends
> > can be configured through the new 'logging' parameter, e.g.:
> > 
> > logging = lttng:10 file:5 syslog:0 
> > 
> > Additional backends are the new interface to the systemd journal that
> > can log more attributes for each message, and for gpfs. The existing
> > config parameters 'syslog' and 'syslog only' are marked as 'deprecated'
> > for now. Once the 'logging' parameter has been  around for a while, the
> > previous parameters could be removed.
> > 
> > Any review and comments would be appreciated.
> 
> I've browsed through most of this and it looks very nice.
> 
> Can I please offer the following potential improvements?
> 
> The debug code is very heavily used so I would prematurely
> optimise.  :-)  Instead of using an array of all possible backends in
> debug_set_backends() and debug_backends_log(), I would suggest using a
> doubly-linked list, and inserting the backends in reverse-sorted order
> of priority.  Then you only need to traverse the list as long as
> msg_level <= log_level.  This makes debug_set_backends() more expensive
> but makes debug_backends_log() cheaper.

If we decide to optimze this case, i would prefer Volker's idea. See my
response to his email.

> On top of this change, since you wouldn't need to reset the log level
> for all backends, you could also tweak the reload() API.  I think that
> if the reload() function had access to the backend structure (e.g.
> "self") then it could just be passed the new log_level, do the right
> magic (since it knows whether it was active before and knows the old log
> level) and then update the log level to the new one. This would probably
> involve moving the list of (previously) active backends to a temporary
> variable, putting each continuing/new one into the active list and
> calling reload().  Any leftovers could just have -1 passed to reload()
> as the log level and would close/clean-up.

Hmm... The question is whether the code should have a linked list at
all as the optimized array might give better performance.

> Given that this would mean passing the log level to reload(), you might
> as well pass the token.  That also gives you the possibly of passing
> options to the backend.  Changing the log level separator to '@', you
> could do things like "syslog:nonblocking at 1 file:/var/log/log.smbd at 3".
> In CTDB I implemented some of the option handling by making reload()
> set the callback or, in this case, backend.log.  If we can also do
> option handling then we can move some extra features from the CTDB
> syslog code into debug.c.
> 
> All of the above would allow you to drop new_log_level from struct
> debug_backend and reload would look like:
> 
>   void (*reload)(struct debug_backend *self, char *tok, const char *prog_name);
> 
> Crazy?  :-)

Not crazy, :-) but i think the question is what do we want to implement
now, and what is coming later. The syntax of the 'logging' parameter is
currently similar to the one in 'log level'. In the long-term, i would
like to see the ctdb logging code to be merged with the Samba code, and
have all backends available in all components. I think the important
piece now would be agreeing on the syntax for specifying options.

With your suggestion, the syntax would be:

name[:option][@level]

with 'option' and 'level' being both optional. If nobody objects, i can
update the patches to accomodate for that, but none of the currently
used backends will use the option field. Once we get around to move the
ctdb logging backends, they would make use of the option field.

Does that sound like a plan?

Christof


More information about the samba-technical mailing list