mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <>
To: Matheus Tavares <>
Cc: git <>,
	"brian m. carlson" <>,
	Johannes Sixt <>,
	Jonathan Tan <>,
	Jeff King <>,
	Johannes Schindelin <>
Subject: Re: [PATCH 0/2] Make oid_to_hex() thread-safe
Date: Fri, 26 Jun 2020 10:22:41 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
<> wrote:
> Some thread-unsafe functions of our codebase appear very down in the
> call stack, which can be hard to notice (or avoid). Thus they are
> sometimes used in threaded code unsafely. In this series we add
> pthread_once() to compat/win32/ and use it in conjunction with
> pthread_key to make a subset of the said functions thread-safe.


> As a next step, I would love to make [warning|error|die]_errno()
> thread-safe as well. strerror() is not safe on *nix, and there are some
> thread functions today that call these (although the actual risk of a
> race condition must be very small...)
> My idea was to implement a xstrerror() wrapper which calls the
> appropriate thread-safe function (dependant on the OS),

Yeah, that works if there are appropriate thread-safe functions for
all the OS we are interested in, or if we can fallback to strerror()
or calling it holding a lock.

> or even call
> strerror() itself but holding a lock to copy the result for a local
> buffer (which should be OK as we don't expect contention in strerror).

I agree that it should be ok.

> 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.

> One concern with this idea is the risk of an infinite recursion if
> xstrerror() or any of its childs call [warning|error|die]_errno().
> However, if we are only using strerror() and pthread_*() within the
> wrapper, there should be no problem, right?

Yeah, I agree.

> Has anyone thought of
> other problems with this approach?
> 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.


  parent reply	other threads:[~2020-06-26  8:22 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       ` Christian Couder [this message]
2020-06-26 16:22         ` [PATCH 0/2] Make oid_to_hex() thread-safe Matheus Tavares Bernardino
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:

  List information:

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

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \

* 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

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).