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.
Done.
> [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.
Well, the value isn't really big.
-1 is 0xFFF... so you have to check against bit map like this:
(debug_level >= 0) && (level & debug_level)
...and I've been told to keep that macro as small as possible. But I'm
aware that it adds more complexity to the code so feel free to refuse
this change.
Also, wouldn't this
conflict with compatibility with old settings if 'debug_level = 0' had
been explicitly set?
Actually, it wouldn't conflict. If you try to put unresolved value in
debug_level, it will be converted to 0x0010 (equiv. to 0 in old format).
It makes problem only if you need to know if the program is run with -d
0 (so we can call every child with this parameter too) or not - and
that's where the above pattern come in.