[SSSD] [PATCHES] New DEBUG facility
Stephen Gallagher
sgallagh at redhat.com
Mon Aug 8 13:28:44 UTC 2011
On Mon, 2011-08-08 at 07:51 -0400, Pavel Brezina wrote:
> https://fedorahosted.org/sssd/ticket/925
>
> [PATCH 1/2] There are several new macros in util/util.h:
>
> - SSSDBG_* to reflect a debug level (same names as in the ticket) -
> please, don't use magic numbers anymore
>
> - DEBUG_MSG(level, function, message) which will format the debug
> message like "(time) [prg_name] [function] (level): message\n"
>
> - DEBUG_IS_SET(level) that you should use to check if the level is
> allowed to be logged You can use it like: if
> (DEBUG_IS_SET(SSSDBG_TRACE_LIBS)) {...}
>
For the most part, this patch is good (one minor nitpick: the comment
around DEBUG_MSG identifies the DEBUG macro, not the DEBUG_MSG macro)
However, it is very difficult to review. Please break this patch up into
several:
1) Creation of the new levels and the macros for converting the old
versions to them.
2) Modification of the DEBUG() macro and creation of the DEBUG_MSG()
macro
3) Conversion of the old DEBUG macros to the new levels
4) The unit tests.
This will make it much more digestible for the reviewer.
> [PATCH 2/2] Changes unresolved debug level value (SSSDBG_UNRESOLVED)
> from -1 to 0 so DEBUG macro could be reduced by one condition. Anyway,
> it has a minor effect, every time you want to load debug_level from
> command line parameters, you have to use following pattern:
>
> /* Set debug level to invalid value so we can deside if -d 0 was
> used. */ debug_level = SSSDBG_INVALID;
>
> pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> while((opt = poptGetNextOpt(pc)) != -1) { ... }
>
> debug_level = debug_level != SSSDBG_INVALID ?
> debug_convert_old_level(debug_level) :
> SSSDBG_UNRESOLVED; /* Debug level should be loaded from config file. */
I don't actually see any value to this patch. Also, wouldn't this
conflict with compatibility with old settings if 'debug_level = 0' had
been explicitly set?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110808/7eeff7c3/attachment.sig>
More information about the sssd-devel
mailing list