Re: [PATCH 0/2] Make oid_to_hex() thread-safe

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Jun 26, 2020, 16:22 UTC
Message-ID: <CAHd-oW4bgEv7w2e-wD5aqje_esHA8-_ZgR9trbJXYJ8-M_4F4g@mail.gmail.com>
In-Reply-To: <CAP8UFD0oWuoYLwgYLbkSvDjV1Ymedd_E2j8iv3QGGitgRVq6=Q@mail.gmail.com>

Hi, Christian

Thanks for the feedback!

On Fri, Jun 26, 2020 at 5:22 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 6 quoted lines

>
> On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
> >
> > My idea was to implement a xstrerror() wrapper which calls the
> > appropriate thread-safe function (dependant on the OS),

[...]

Show 5 quoted lines

> > We could also set a thread local buffer array, as in the second patch of
> > this series, to excuse callers from allocating/freeing memory.
>
> I don't think caller allocating/freeing memory for error strings is a
> performance or code simplification issue.

Yeah, I agree that passing a buffer to xstrerror() should be fine. The problem is that we already have many uses of strerror() (98, according to git-grep), which expect a static buffer in return. So the change would be too big :(

Show 6 quoted lines

> > Finally, should such change also come with a coccinelle patch to replace
> > usages of strerror() with xstrerror()? Or better not, as the change
> > would be too big?
>
> I would agree that it's better to avoid too big changes. I am not sure
> how much we want to automate and check that though.

Another alternative would be to replace the definition of strerror() with a:

#define strerror(errnum) xstrerror(errnum)

This way, all the 98 current strerror() callers would start using the thread-safe version without the need to change them.... However, I don't think I've ever seen such a replacement in our codebase before (besides the banned functions in banned.h). Also, people might expect strerror() to refer to the system version, and perhaps make wrong assumptions about it? I'm not sure which is the best alternative, it's a tricky issue :(