[SSSD] DEBUG macro refactoring v3

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Tue Jan 7 13:31:50 UTC 2014


Hi Jakub,

Thanks a lot for reviewing!

On 01/07/2014 02:33 PM, Jakub Hrozek wrote:
> 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 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.

I think a clearer approach would be to reflow it in a separate patch. I'll see
if I have time for this. I wouldn't mind someone else fixing it, though.

> [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?

No, it was there before my changes, in the original "debug_fn" function, which
body was moved to the new "debug_vprintf". That's why I kept it separate.
However, please squash it, if you think it's right.

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

Thanks Jakub. I'm still open to job offers ;)

Sincerely,
Nick



More information about the sssd-devel mailing list