<div dir="ltr"><div><div>I tried it. Unfortunately it fails for those cases of debugs() which do not use xstrerror, e.g.<br><br>../../src/esi/Element.h: In member function 'virtual bool ESIElement::addElement(ESIElement::Pointer)':<br>../../src/esi/Element.h:64:74: error: unused variable '_squid_errno' [-Werror=unused-variable]<br> debugs(86,5, "ESIElement::addElement: Failed for " << this);<br><br>:(<br></div>I believe putting the burden of saving state to the caller is way too intrusive: a simple count places the number of xstrerror calls at about 260.<br></div>So we're back to square one, the -v2 patch. Do you see any other options?<br><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 12, 2015 at 8:12 AM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 12/07/2015 5:45 p.m., Kinkie wrote:<br>
> On Sun, Jul 12, 2015 at 2:34 AM, Amos Jeffries <<a href="mailto:squid3@treenet.co.nz">squid3@treenet.co.nz</a>> wrote:<br>
><br>
>> On 12/07/2015 5:42 a.m., Kinkie wrote:<br>
>>> Hi all,<br>
>>> this patch saves and restores errno around calls to strftime in<br>
>>> debugLogTime, preventing harmless errors in these calls from clobbering<br>
>>> errno, thus messing up xstrerror() calls within debugs().<br>
>>><br>
>><br>
>> 1) xerrno is a local variable in lots of places in the code. This patch<br>
>> introduces shadowing errors. Please call it debug_xerrno.<br>
>><br>
><br>
> Ok, no problem.<br>
><br>
><br>
>> 2) The snprintf() call directly after restoring errno in the top chunk<br>
>> may alter the original errno in rare cases as well.<br>
>><br>
><br>
> Are you sure? The snprintf manual page doesn't mention that.<br>
><br>
><br>
>> Which is a good example of why we simply can't rely on errno being<br>
>> correct for any length of time in C++. Things such as errno changes by<br>
>> operator<< implementations, print() methods, implicit calls to copy<br>
>> constructors/destructors, and exlicit function/method calls are also<br>
>> possible in ways we are unlikely to be predict.<br>
>><br>
>><br>
>> We are still going to have to do a global removal of xstrerror() anyway.<br>
>><br>
><br>
> I do not object on the technical merit.<br>
> At the same time, there are too many to do it incrementally, and we already<br>
> failed to reach a consensus on flag-day commit for HERE removal. Sure,<br>
> maybe xstrerror()+HERE removal will reach a mass critical enough to support<br>
> the flag-day-commit camp, but I don't see that happening, and we already<br>
> have quite a but of rework due to HERE being left in changed code. We'd end<br>
> up for years with a half-done migration.<br>
><br>
><br>
>> IMO, we should move xstrerror() to Debugs.h and make it use the<br>
>> debug_xerrno local variable instead of errno.<br>
>><br>
><br>
> Maybe we could change debugLogTime to return a std::string instead of a<br>
> const char *, and format it ourselves using a stringbuf instead of using<br>
> strftime. Or we could maybe do something like this:<br>
><br>
> int _squid_errno;<br>
> const char *xstrerror() {<br>
> return xstrerr(_squid_errno);<br>
> }<br>
<br>
</div></div>Since this is in a header the function needs to be explicitly 'inline'.<br>
The variable needs explict extern with debug.cc safe initialization, etc.<br>
<br>
Far simpler and nicer to leave it as a macro:<br>
#define xstrerror() xstrerr(_sauid_errno)<br>
<br>
where _squid_errno as a local to the if-statement scope below.<br>
<span class=""><br>
> #define debugs(SECTION, LEVEL, CONTENT) \<br>
> do { \<br>
> if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \<br>
> Debug::sectionLevel = Debug::Levels[SECTION]; \<br>
> _squid_errno = errno;<br>
> std::ostream &_dbo=Debug::getDebugOut(); \<br>
> if (Debug::level > DBG_IMPORTANT) \<br>
> _dbo << SkipBuildPrefix(__FILE__)<<"("<<__LINE__<<")<br>
> "<<__FUNCTION__<<": "; \<br>
> _dbo << CONTENT; \<br>
> Debug::finishDebug(); \<br>
> } \<br>
> } while (/*CONSTCOND*/ 0)<br>
><br>
> This would make xstrerror only work within debugs(); maybe we could add a<br>
> guard against it by setting _squid_errno to some invalid value and checking<br>
> it in xstrerror. Yes, it's ugly.<br>
<br>
</span>Yes, which is why I requested xstrerror() move to Debug.h as part of the<br>
change. The code not using debugs() can and should use xstrerr(foo) instead.<br>
<span class=""><br>
><br>
> But until we reach a consensus, I'd like authorization to move forward with<br>
> the revised patch I'm attaching. It's far from ideal, but it's a step.<br>
><br>
<br>
</span>IIRC we already have agreement on removal of xstrerror() globally. Its<br>
not as large as HERE removal and will fix many hidden bugs.<br>
<br>
Moving xstrerror() to a debugs-internal mechanism will actually reduce<br>
the size of that change. Which is why I'm continuing with this thread of<br>
discussion at all.<br>
<span class="HOEnZb"><font color="#888888"><br>
Amos<br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"> Francesco</div>
</div>