[PATCHES] Logging to multiple debug backends

[Martin Schwenke]

On Mon, 16 Mar 2015 14:07:31 -0700, Christof Schmitt <cs at samba.org>
wrote:

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

It does.  Sold!  :-)

peace & happiness,
martin