[SSSD] DEBUG macro refactoring v3

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Fri Dec 20 20:01:12 UTC 2013


Hi Stephen,

Thanks for the review! Replies below.

On 12/20/2013 09:31 PM, Stephen Gallagher wrote:
> 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.

Sure, these were made separate with the intention to make everyone aware that
the script was non-ideal and to make it easier to re-run it if the code
changes before it is acked. It is better to submit these as a single commit
and the message above will do fine.

> 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.

Yes, I wanted to comment on line lengths, but then forgot about it, sorry.

It might be tricky to do automatically and might harm more if done
incorrectly, but let me try something. Maybe I'll be able to improve it for
most cases.

> 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.
>
> So, technical Ack, with reservations.

Hmm, yes it might happen. I'll leave this up to you, of course.

Sincerely,
Nick



More information about the sssd-devel mailing list