[squid-dev] [PATCH] avoid clobbering errno in debugs()

[Kinkie]

I tried it. Unfortunately it fails for those cases of debugs() which do not
use xstrerror, e.g.

../../src/esi/Element.h: In member function 'virtual bool
ESIElement::addElement(ESIElement::Pointer)':
../../src/esi/Element.h:64:74: error: unused variable '_squid_errno'
[-Werror=unused-variable]
         debugs(86,5, "ESIElement::addElement: Failed for " << this);

:(
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.
So we're back to square one, the -v2 patch. Do you see any other options?


On Sun, Jul 12, 2015 at 8:12 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 12/07/2015 5:45 p.m., Kinkie wrote:
> > On Sun, Jul 12, 2015 at 2:34 AM, Amos Jeffries <squid3 at treenet.co.nz>
> wrote:
> >
> >> On 12/07/2015 5:42 a.m., Kinkie wrote:
> >>> Hi all,
> >>>   this patch saves and restores errno around calls to strftime in
> >>> debugLogTime, preventing harmless errors in these calls from clobbering
> >>> errno, thus messing up xstrerror() calls within debugs().
> >>>
> >>
> >> 1) xerrno is a local variable in lots of places in the code. This patch
> >> introduces shadowing errors. Please call it debug_xerrno.
> >>
> >
> > Ok, no problem.
> >
> >
> >> 2) The snprintf() call directly after restoring errno in the top chunk
> >> may alter the original errno in rare cases as well.
> >>
> >
> > Are you sure? The snprintf manual page doesn't mention that.
> >
> >
> >> Which is a good example of why we simply can't rely on errno being
> >> correct for any length of time in C++. Things such as errno changes by
> >> operator<< implementations, print() methods, implicit calls to copy
> >> constructors/destructors, and exlicit function/method calls are also
> >> possible in ways we are unlikely to be predict.
> >>
> >>
> >> We are still going to have to do a global removal of xstrerror() anyway.
> >>
> >
> > I do not object on the technical merit.
> > 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.
> >
> >
> >> IMO, we should move xstrerror() to Debugs.h and make it use the
> >> debug_xerrno local variable instead of errno.
> >>
> >
> > 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:
> >
> > int _squid_errno;
> > const char *xstrerror() {
> >   return xstrerr(_squid_errno);
> > }
>
> Since this is in a header the function needs to be explicitly 'inline'.
> The variable needs explict extern with debug.cc safe initialization, etc.
>
> Far simpler and nicer to leave it as a macro:
>  #define xstrerror() xstrerr(_sauid_errno)
>
> where _squid_errno as a local to the if-statement scope below.
>
> > #define debugs(SECTION, LEVEL, CONTENT) \
> >    do { \
> >         if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
> >             Debug::sectionLevel = Debug::Levels[SECTION]; \
> >             _squid_errno = errno;
> >             std::ostream &_dbo=Debug::getDebugOut(); \
> >             if (Debug::level > DBG_IMPORTANT) \
> >                 _dbo << SkipBuildPrefix(__FILE__)<<"("<<__LINE__<<")
> > "<<__FUNCTION__<<": "; \
> >             _dbo << CONTENT; \
> >             Debug::finishDebug(); \
> >         } \
> >    } while (/*CONSTCOND*/ 0)
> >
> > 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.
>
> Yes, which is why I requested xstrerror() move to Debug.h as part of the
> change. The code not using debugs() can and should use xstrerr(foo)
> instead.
>
> >
> > 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.
> >
>
> IIRC we already have agreement on removal of xstrerror() globally. Its
> not as large as HERE removal and will fix many hidden bugs.
>
> Moving xstrerror() to a debugs-internal mechanism will actually reduce
> the size of that change. Which is why I'm continuing with this thread of
> discussion at all.
>
> Amos
>



-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150712/dfcd914d/attachment.html>