[SSSD] [PATCHES] New DEBUG facility

Pavel Březina pbrezina at redhat.com
Wed Aug 17 07:48:16 UTC 2011


Dne 16.8.2011 15:20, Jan Zelený napsal(a):
>> Rebased to current master
>> (2011-08-15 17:46 Jakub Hrozek 5bf2314b9f64099cd4e88b8f3498d986d97e1ac6)
>
> I have some questions and comments. In general, the patches look fine, but
> there are couple things (mostly cosmetic) which I'd like to get right.
>
> How about mask (#define) adding only failures / only traces / ... ? Dou you
> think it would make sense to add them or not?

I don't think it would be valuable for developers - you don't usually 
set debug_level manually. But user would definitely appreciate some 
aliases (like -d all, -d failures, etc.), maybe in another patch.

>
> Commit comments - the same comment in more patches is a little confusing.
> Please leave only the part relevant for the patch.
>
>
> Patch #0001:
> - why are there two functions handling almost the same thing differently?
>    (debug_convert_old_level and debug_get_level)

It's actually not same at all.
debug_convert_old_level() is used for user input. When he provides -d 5, 
it will convert it to 0x03F0 (= 0 | 1 | 2 | 3 | 4 | 5).

debug_get_level() is used for developer input. When you call 
DEBUG(5,...) it will return 0x0200 (= 5).

> Patch #0002:
> - lines 45, 76, 96: the first part of condition is redundant
>    (removed in patch #0006 - please merge that part with this patch)

The condition is not redundant - -1 (unresolved value) = 0xfff... and in 
the way how the code looks in this patch, it may happen that debug_level 
== -1 when DEBUG() macro is called.

> - please use DEBUG_IS_SET in DEBUG_MSG and DEBUG

Good idea.


> Patch #0003:
> - line 166: I'd like to avoid changing these DEBUG statements like this
>    -- either change them all or none, not just the one
>    -- the same for lines 212, 228, 313
>
> Patch #0004:
> - I have to read through it yet, perhaps a native english speaker should take
> a look as well
>
> Patch #0005:
> - I'll trust you on this one if all tests pass ;-)
>
> Patch #0006:
> -<nitpick>  please just for safety use parentheses in ternary operator (line
> 92 and couple more similar)</nitpick>

I probably should change it to a macro as well.

> - why don't you remove SSS_UNRESOLVED_DEBUG_LEVEL entirely?

I didn't realize that all references are gone :-)

> I'm running some more tests now, but I think I won't find any further issues.
>
> Jan




More information about the sssd-devel mailing list