[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