<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Jul 12, 2015 at 2:34 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">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>
</span>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></blockquote><div><br></div><div>Ok, no problem.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
2) The snprintf() call directly after restoring errno in the top chunk<br>
may alter the original errno in rare cases as well.<br></blockquote><div><br></div><div>Are you sure? The snprintf manual page doesn't mention that.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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></blockquote><div><br></div><div>I do not object on the technical merit. <br>At the same time, there are too many to do it incrementally, and we already failed to reach a consensus on flag-day commit for HERE removal. Sure, maybe xstrerror()+HERE removal will reach a mass critical enough to support the flag-day-commit camp, but I don't see that happening, and we already have quite a but of rework due to HERE being left in changed code. We'd end up for years with a half-done migration.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
IMO, we should move xstrerror() to Debugs.h and make it use the<br>
debug_xerrno local variable instead of errno.<br></blockquote><div><br></div><div>Maybe we could change debugLogTime to return a std::string instead of a const char *, and format it ourselves using a stringbuf instead of using strftime. Or we could maybe do something like this:<br></div><div><br></div><div>int _squid_errno;<br></div><div>const char *xstrerror() {<br></div><div>  return xstrerr(_squid_errno);<br></div><div>}<br></div>#define debugs(SECTION, LEVEL, CONTENT) \<br>   do { \<br>        if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \<br>            Debug::sectionLevel = Debug::Levels[SECTION]; \<br></div><div class="gmail_quote">            _squid_errno = errno;<br></div><div class="gmail_quote">            std::ostream &_dbo=Debug::getDebugOut(); \<br>            if (Debug::level > DBG_IMPORTANT) \<br>                _dbo << SkipBuildPrefix(__FILE__)<<"("<<__LINE__<<") "<<__FUNCTION__<<": "; \<br>            _dbo << CONTENT; \<br>            Debug::finishDebug(); \<br>        } \<br>   } while (/*CONSTCOND*/ 0)<br><br><div>This would make xstrerror only work within debugs(); maybe we could add a guard against it by setting _squid_errno to some invalid value and checking it in xstrerror. Yes, it's ugly.<br><br></div><div>But until we reach a consensus, I'd like authorization to move forward with the revised patch I'm attaching. It's far from ideal, but it's a step.<br clear="all"></div></div><br>-- <br><div class="gmail_signature">    Francesco</div>
</div></div>