[SSSD] DEBUG macro refactoring v3

Jakub Hrozek jhrozek at redhat.com
Tue Jan 7 12:33:21 UTC 2014


On Wed, Dec 25, 2013 at 02:10:20AM +0200, Nikolai Kondrashov wrote:
> On 12/23/2013 01:42 PM, Jakub Hrozek wrote:
> >On Sun, Dec 22, 2013 at 11:23:58AM +0200, Nikolai Kondrashov wrote:
> >>
> >>     1. There are 2273 uses of the old levels, *half* of all DEBUG macro
> >>        invocations, so updating them on occasion will take a *long* time.
> >>     2. Code is more often read than written (modified), and all this time
> >>        developers will be reading code using obscure numeric literals, which
> >>        take additional mental hop (effort) to comprehend.
> >>     3. Spotting incorrect debug level will be easier with symbolic levels
> >>        instead of numeric ones and thus they're more likely to be corrected.
> >
> >Sorry, I disagree about 2) and 3). Maybe it's just how I work, but I've
> >trained myself to convert any numeric constants I see in the code and
> >just skip the SSSDBG_ unless I'm also touching the DEBUG macro for other
> >reasons.
> 
> The new debug levels were introduced at the beginning of August 2011
> (a32ae272). That makes it about 2.5 years ago. Even assuming update progress
> is linear (which it isn't), it will take 2.5 years more to update the other
> half. That means 2.5 years of reading code with extra effort for developers.
> Although, admittedly, the less old code there is, the less effort there will
> be (but that is not linear as well).
> 
> Sincerely,
> Nick

I spoke about the same problem with Simo (or Stephen? Sorry, I forgot
over holidays) and it seems my opinion is in minority so I won't block
the patchset over it.

I have some very minor comments about the patches, see below:

[PATCH 1/8] Add cscope inverted index files to .gitignore
ACK


[PATCH 2/8] Move DEBUG macro body to debug_fn
Mostly ACK, I just have a question about debug_fn() -- while I realize
the code was taken from the old DEBUG macro, now that it's a function,
maybe you could reflow it so it's more in line with our coding style and
better readable as a result. In particular, there could be a newline after
declaration of "va_list ap" and before call to "va_start".

Similarly, the block executed after testing debug_timestamps() could be its
own functions, as in general we don't do intermixed code and declarations.

But this is mostly my personal preference, again, not blocking the patches.


[PATCH 3/8] Remove extra flushing from debug message output
The code is fine, but why is it a separate patch and not part of #2 ? It
seems the issue was introduced in patch 2/8, right?


[PATCH 4/8] Make DEBUG macro definition variadic
ACK


[PATCH 5/8] Make DEBUG macro invocations variadic
ACK, although I haven't reviewed the patch too closely, I simply checked
couple of random messages.


[PATCH 6/8] Fixup DEBUG macro invocations update
ACK


[PATCH 7/8] Update DEBUG* invocations to use new levels
ACK (I tested the new version that was sent separately)


[PATCH 8/8] Remove debug_get_level as unnecessary
ACK

I'm not too afraid about breaking 3rd party patches. We can announce
this change on the devel list separately (w/o the [PATCH] keyword) and
the patches would break at build-time, so the breakage wouldn't affect
users, only packagers. They should be able to come for help if they
can't fix the patch themselves.

So in conclusion - ACK, I'll leave up to you whether you want to change
formatting of debug_fn and/or squash 3/8 into 2/8.

Great work!



More information about the sssd-devel mailing list