Re: [PATCH] debug prints

On 19/02/2013 9:22 a.m., Alex Rousskov wrote:
> On 02/18/2013 04:39 AM, Amos Jeffries wrote:
>> On 10/08/2012 1:57 p.m., Amos Jeffries wrote:
>>> On 9/08/2012 12:12 a.m., Alexander Komyagin wrote:
>>>> Following patch fixes nested debug() calls problem.
>>>> Since debugs() is a macros, it should not change static Debugs::level
>>>> before putting debug message to the internal stream. Otherwise we
>>>> encounter problems when debug message itself contains calls to debugs().
>>>>
>>>> --- src/Debug.h 2012-03-07 05:42:55.000000000 +0300
>>>> +++ src/Debug.h 2012-08-08 14:49:20.000000000 +0400
>>>> @@ -106,8 +106,9 @@
>>>> /* Debug stream */
>>>> #define debugs(SECTION, LEVEL, CONTENT) \
>>>> do { \
>>>> - if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
>>>> + if ((LEVEL) <= Debug::Levels[SECTION]) { \
>>>> Debug::getDebugOut() << CONTENT; \
>>>> + Debug::level = (LEVEL); \
>>>> Debug::finishDebug(); \
>>>> } \
>>>> } while (/*CONSTCOND*/ 0)
>>>>
>>> Looks okay. How much testing has this had?
>>>
>>> Amos
>> Applied as trunk rev.12698. Sorry this took so long to go in.
> Please note that debugs() is not the only macro that changes
> Debug::level before doing anything else. The old do_debug() does that as
> well.
>
> debugs() still changes Debug::sectionLevel before we do anything else so
> there is inconsistency now with regard to level and sectionLevel.
>
> Finally, this change makes current debugging level unavailable to
> CONTENT. I know some CONTENT uses Debug::sectionLevel. I do not see any
> CONTENT using Debug::level, so this may not be a problem today.
>
> I cannot tell exactly what problems this change is fixing (the commit
> message does not explain), so the fix may very well be worth the
> side-effects, but it looks like the debugging code became messier after
> this change. I wonder if the fix is worth it and whether there is a
> better way to fix that problem?

Possibly. Reverting. I applied because it passed the audit conditions
months back but had not been applied by anyone yet.

Amos
Received on Mon Feb 18 2013 - 23:34:14 MST