[SSSD] [PATCHES] New DEBUG facility

Pavel Březina pbrezina at redhat.com
Tue Aug 9 12:11:49 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.

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.

Pavel Březina
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-New-DEBUG-facility-new-levels.patch
Type: text/x-patch
Size: 4457 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-New-DEBUG-facility-modified-DEBUG.patch
Type: text/x-patch
Size: 4094 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-New-DEBUG-facility-conversion.patch
Type: text/x-patch
Size: 23679 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-New-DEBUG-facility-man-pages.patch
Type: text/x-patch
Size: 4998 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-New-DEBUG-facility-unit-tests.patch
Type: text/x-patch
Size: 22258 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-New-DEBUG-facility-SSSDBG_UNRESOLVED-changed-from-1-.patch
Type: text/x-patch
Size: 28126 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110809/9f51d3a2/attachment-0005.bin>


More information about the sssd-devel mailing list