git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
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 10:22:41 +0200	[thread overview]
Message-ID: <CAP8UFD0oWuoYLwgYLbkSvDjV1Ymedd_E2j8iv3QGGitgRVq6=Q@mail.gmail.com> (raw)
In-Reply-To: <cover.1593115455.git.matheus.bernardino@usp.br>

On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
<matheus.bernardino@usp.br> 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.

Great!

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

Thanks,
Christian.

  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:
  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='CAP8UFD0oWuoYLwgYLbkSvDjV1Ymedd_E2j8iv3QGGitgRVq6=Q@mail.gmail.com' \
    --to=christian.couder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=matheus.bernardino@usp.br \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH 0/2] Make oid_to_hex() thread-safe' \
    /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

Code repositories for project(s) associated with this 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).