Re: LDAP Debug messages & severity levels

[Pierangelo Masarati <ando@sys-net.it>]

On Fri, 2006-01-20 at 22:15 +0100, Hallvard B Furuseth wrote:
> Replying to old mail...
> 
> Pierangelo Masarati writes:
> > Hallvard,
> >
> > did you get any far with your logging redesign?
> 
> It stalled on something or other I was waiting for, but here is what I
> was thinking of.  Somewhat at odds with what you have committed, I don't
> know whether to blend the two or stick to one of them:

Well, it shouldn't be too difficult.


> ldap_log.h:
> 
>   /* Helper macro for unconditional logging - Fatal, Error, Warn. */
>   #define LDAP_LOGU( level, args ) do { \
> 	do { \
> 		lutil_debug( ldap_debug, -1, args ); \
> 		syslog( LDAP_LEVEL_MASK((severity)), args ); \
> 	} while ( 0 )

This would be an addition with respect to the current logging, so it
doesn't interfere too much.

> 
>   /* Helper macro for conditional logging - Info, Error. */
>   #define LDAP_LOGC( level, logmask, args ) do { \
> 	do { \
> 		if ( ldap_debug & (logmask) ) \
> 			lutil_debug( ldap_debug, (logmask), args ); \
> 		if ( ldap_syslog & (level) ) \
> 			syslog( LDAP_LEVEL_MASK((severity)), args ); \
> 	} while ( 0 )
> 
>   /* Used to send several arguments to one LDAP_LOG[UC] argument */
>   #define LDAP_ARG ,

I don't like this approach too much; in general, I think that trying to
emulate variable arg number in macros leads to confusion.  I prefer to
waste time writing several macros for each number of args (up to a
reasonable amount; the time we need more we can add them...)

> 
>   /* Logging API */
> 
>   /* Conditional logging, and only compiled in when configured */
>   #define Debug Debug3
>   #ifdef LDAP_DEBUG /* or should that test LDAP_DEBUG || LDAP_SYSLOG? */
>   #define Debug0( logmask, fmt ) \
> 	LDAP_LOGC( LDAP_LEVEL_DEBUG, logmask, (fmt) )
>   #define Debug1( dummy, fmt, arg1 ) ...
>   #define Debug2( dummy, fmt, arg1, arg2 ) ...
>   #define Debug3( dummy, fmt, arg1, arg2, arg3 ) \
> 	LDAP_LOGU( LDAP_LEVEL_DEBUG, logmask, \
> 	(fmt) LDAP_ARG(arg1) LDAP_ARG(arg2) LDAP_ARG(arg3) )
>   ...
>   #else
>   #define Debug0( logmask, fmt ) ((void) 0)
>   ...
>   #endif
> 
>   /* Conditional logging, always compiled in - because Statslog()
>    * will be like Info() and will always be compiled in. */

I think we can safely remove Statslog (that's what I did) as there is no
difference between it and regular logging; it used to require conn & op,
which makes little sense as it was being used as a Debug() with two more
args...

>   #define Info Info3
>   #define Info0( logmask, fmt ) \
> 	LDAP_LOGC( LDAP_LEVEL_INFO, logmask, (fmt) )
>   ...
> 
>   /* Unconditional logging.
>   /* The dummy argument is unused and can be removed later, but
>    * is included for now so we can rename Fatal<->...<->Debug at
>    * need without having to add/remove the logmask argument. */
>   #define Warn Warn3
>   #define Warn0( dummy, fmt )  LDAP_LOGU( LDAP_LEVEL_WARNING, (fmt) )
>   ...
>   #define Error Error3
>   #define Error0( dummy, fmt ) LDAP_LOGU( LDAP_LEVEL_ERR, (fmt) )
>   ...
>   #define Fatal Fatal3
>   #define Fatal0( dummy, fmt ) LDAP_LOGU( LDAP_LEVEL_CRIT, (fmt) )
>   ...
> 
> I've taken the opportunity to rename loglevel to logmask, which more
> accurately describes what it is.

As I said, I prefer to have one macro with an arg that identifies the
severity rather than multiple macros, one for each severity, but it's a
matter of taste.  We can always wrap one approach into the other,
although there would be no clear advantage and things might even get
more confusing.

To summarize:
- I agree in differentiating unconditional and conditional logging;
unconditional logging would be the equivalent of Log
( LDAP_DEBUG_ANY, ...).
- I like the logmask idea because it's very esplicative.
- I don't like the variable arg number, but it's personal taste, so I'd
let consensus gather before we decide.
- I prefer to have few macros with severity as an arg rather than in the
macro name.

p.




Ing. Pierangelo Masarati
Responsabile Open Solution
OpenLDAP Core Team

SysNet s.n.c.
Via Dossi, 8 - 27100 Pavia - ITALIA
http://www.sys-net.it
------------------------------------------
Office:   +39.02.23998309          
Mobile:   +39.333.4963172
Email:    pierangelo.masarati@sys-net.it
------------------------------------------