[SSSD] [PATCHES] New DEBUG facility

Jan Zelený jzeleny at redhat.com
Wed Aug 17 13:29:30 UTC 2011


> > Commit comments - the same comment in more patches is a little confusing.
> > Please leave only the part relevant for the patch.
> 
> Done.
> 
> > Patch #0002:
> > - lines 45, 76, 96: the first part of condition is redundant
> > 
> >     (removed in patch #0006 - please merge that part with this patch)
> 
> Merging would be really difficult, so I'm leaving it separated if it's
> not necessary.

I'd prefer if it wasn't but since all the patches will be applied for this to 
work, I'm willing to overlook this ;-)

> > - please use DEBUG_IS_SET in DEBUG_MSG and DEBUG
> 
> Done.
> 
> > 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
> 
> Done.
> Also removed SSS_DEFAULT_DEBUG_LEVEL (replaced with SSSDBG_DEFAULT).
> 
> > Patch #0006:
> > -<nitpick>   please just for safety use parentheses in ternary operator
> > (line 92 and couple more similar)</nitpick>
> 
> Done and also moved to macro.
> I've created two new macros so the pattern to load debug_level from
> command line is now:
> 
>          /*
>           * Set debug level to invalid value
>           * so we can deside if -d 0 was used.
>           */
>          INVALIDATE_DEBUG_LEVEL();
> 
>          pc = poptGetContext(argv[0], argc, argv, long_options, 0);
>          while((opt = poptGetNextOpt(pc)) != -1) { ... }
> 
>          CONVERT_AND_SET_DEBUG_LEVEL(debug_level);
> > - why don't you remove SSS_UNRESOLVED_DEBUG_LEVEL entirely?
> 
> Removed (replaced with SSSDBG_UNRESOLVED).

You still don't have parentheses around the condition in ternary operator. No 
big deal though, it doesn't concern me in the macro since bringing an error to 
one possible place is less likely than to one of many possible places.

One more thing: I don't like the invalidate macro. The assignment with the 
comment you provided is clear enough I think.

Other than this one small thing, the patches are ok.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110817/87d663b1/attachment.sig>


More information about the sssd-devel mailing list