On Tue, Jan 07, 2014 at 03:31:50PM +0200, Nikolai Kondrashov wrote: > 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.
Ah, sorry, my review was flawed. You're right. Nevertheless, I think I found one bug in patch #2 when testing Stephen's follow-up patches. I don't think it's correct to replace all occurences of DEBUG_MSG with debug_fn. The reason is that DEBUG_MSG used to perform check if a particular level is set, debug_fn prints directly. One example of where we need to check the debug level is ldb_debug_messages(), with the current version of the patch, the ldb messages are always printed. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
