git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Christian Couder <christian.couder@gmail.com>
Cc: git <git@vger.kernel.org>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Johannes Sixt <j6t@kdbg.org>,
	Jonathan Tan <jonathantanmy@google.com>,
	Jeff King <peff@peff.net>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 0/2] Make oid_to_hex() thread-safe
Date: Fri, 26 Jun 2020 13:22:52 -0300	[thread overview]
Message-ID: <CAHd-oW4bgEv7w2e-wD5aqje_esHA8-_ZgR9trbJXYJ8-M_4F4g@mail.gmail.com> (raw)
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:
>
> 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),
[...]
> > 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 :(

> > 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 :(

  reply	other threads:[~2020-06-26 16:23 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 22:29 [RFC] Thread safety in some low-level functions Matheus Tavares Bernardino
2020-06-24 22:52 ` Matheus Tavares Bernardino
2020-06-25  1:38   ` brian m. carlson
2020-06-25 20:32     ` [PATCH 0/2] Make oid_to_hex() thread-safe Matheus Tavares
2020-06-25 20:32       ` [PATCH 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26  5:45         ` Christian Couder
2020-06-26 14:13           ` Matheus Tavares Bernardino
2020-06-25 20:32       ` [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-25 23:07         ` brian m. carlson
2020-06-25 23:54           ` Matheus Tavares Bernardino
2020-06-26  0:00             ` Matheus Tavares Bernardino
2020-06-26  6:02         ` Christian Couder
2020-06-26  8:22       ` [PATCH 0/2] Make oid_to_hex() thread-safe Christian Couder
2020-06-26 16:22         ` Matheus Tavares Bernardino [this message]
2020-06-26 21:54       ` [PATCH v2 " Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-29 15:11           ` Johannes Schindelin
2020-06-30 20:37             ` Matheus Tavares Bernardino
2020-07-01 11:32               ` Johannes Schindelin
2020-07-16 11:29           ` Johannes Schindelin
2020-07-18  3:09             ` Matheus Tavares Bernardino
2020-08-10 14:15               ` Johannes Schindelin
2020-07-18  3:52             ` Matheus Tavares
2020-07-26 17:41               ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHd-oW4bgEv7w2e-wD5aqje_esHA8-_ZgR9trbJXYJ8-M_4F4g@mail.gmail.com \
    --to=matheus.bernardino@usp.br \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).