[SSSD] DEBUG macro refactoring v3

Stephen Gallagher sgallagh at redhat.com
Fri Dec 20 19:43:16 UTC 2013


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/20/2013 02:31 PM, Stephen Gallagher wrote:
> On 12/20/2013 11:15 AM, Nikolai Kondrashov wrote:
>> Hi everyone,
> 
>> Please find attached the third version of the DEBUG macro 
>> refactoring patchset. The second version was lost to the
>> maillist message size limit, so I'll list the changes from the
>> first one:
> 
>> * level limiting condition moved to the macro as requested by 
>> Simo, * macro definition update is separated from the invocation 
>> update, * automatic macro invocation update is separated from
>> the following manual fixups, * invocations using the old debug
>> level (number literals) are updated to use the new bitmask
>> macros, * the level conversion function "debug_get_level" is
>> removed.
> 
>> Please feel free to pick and squash commits as necessary.
> 
>> Sincerely, Nick
> 
> Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack
> 
> Patch 0005: Nack Patch 0006: Nack
> 
> I'd rather that 0005 and 0006 be squashed together and that the
> comment be written as follows:
> 
> {{{ Make DEBUG macro invocations variadic
> 
> Use a script to update DEBUG macro invocations to use it as a
> variadic macro, supplying format string and its arguments directly,
> instead of wrapping them in parens.
> 
> This script was used to update the code:
> 
> grep -rwl --include '*.[hc]' DEBUG . | while read f; do mv
> "$f"{,.orig} perl -e \ 'use strict; use File::Slurp; my
> $text=read_file(\*STDIN); 
> $text=~s#(\bDEBUG\s*\([^(]+)\((.*?)\)\s*\)\s*;#$1$2);#gs; print
> $text;' < "$f.orig" > "$f" rm "$f.orig" done
> 
> There exists a single non-automated change in this patch, due to
> the script being slightly too greedy in two places.
> 
> 1. The definition of the DEBUG() macro matched incorrectly, causing
> the parser to greedily continue advancing until it eventually found
> a double-closing-parenthesis in the talloc_zfree() macro
> definition.
> 
> 2. The presence of a comment containing the literal string
> "DEBUG()" caused the parser to incrorectly match and proceed to the
> next valid DEBUG() macro usage, removing the trailing parenthesis
> but missing the leading one. }}}
> 
> That being said, the logic seems correct everywhere else. Nikolai,
> if you agree with this, no need to resend the patches. This is
> easily adjusted before we push, so you can consider it an Ack.
> 
> 
> Patch 0007: The conversion seems *correct*, if not ideal. I don't
> love that it effectively makes all of our DEBUG lines exceed our
> 79-character limit. I don't suppose you could amend your patch to
> reflow things using our current convention:
> 
> DEBUG(SSSDBG_TRACE_ALL, "Some words go here\n", arg1, arg2);
> 
> I'm not willing to block the patch on this, however. Particularly
> since a non-trivial number of existing DEBUG messages were already
> over the limit.
> 
> One other comment I seem to recall from the earlier (long) thread
> that I'd like to address. It was brought up that the reason we were
> leaving these alone was to deal with them only when we touched the
> code they to which they were related. I think by doing this
> mass-conversion, it becomes much easier to spot at a glance when a
> message is poorly categorized. It makes it possible for a new
> contributor to come in and fix up these levels, even when not
> working on other code in the area.
> 
> So I'm in favor of making this change, and best to do it all at
> once and rip off the band-aid.
> 
> Consider 0007 acked, unless Nikolai wants to try his hand at
> reflowing it.
> 
> Patch 0008: I'm not sure we want to drop this compatibility
> without checking with all of our downstream packagers (and anyone
> who is building with custom patches). It's very possible that they
> may be carrying forward old patches that still retain the 0-9 debug
> level values and breaking them might be unwelcome.
> 
> At the very least, this would need to be documented loudly in the 
> release notes for the version in which this change occurs.
> 

Actually, minor Nack on 0008:

 #define DEBUG(level, format, ...) do { \
- -    int __debug_macro_newlevel = debug_get_level(level); \
- -    if (DEBUG_IS_SET(__debug_macro_newlevel)) \
- -        debug_fn(__FUNCTION__, __debug_macro_newlevel, format,
##__VA_ARGS__); \
+    int __debug_macro_level = level; \
+    if (DEBUG_IS_SET(__debug_macro_level)) \
+        debug_fn(__FUNCTION__, __debug_macro_level, format,
##__VA_ARGS__); \
 } while (0)


If you're going to drop debug_get_level(), there's no reason to use
_debug_macro_level at all here. Drop that too.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.15 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlK0ndQACgkQeiVVYja6o6MaaQCeNCbb6fJSgRI5rkSljJj8YrKv
AAIAnjmU04UxvgDjMpH4RKNJk/vqYlhJ
=xlck
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list