From: Christian Couder <email@example.com> To: Matheus Tavares <firstname.lastname@example.org> Cc: git <email@example.com>, "brian m. carlson" <firstname.lastname@example.org>, Johannes Sixt <email@example.com>, Jonathan Tan <firstname.lastname@example.org>, Jeff King <email@example.com>, 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: <firstname.lastname@example.org> On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares <email@example.com> 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.
next prev 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' \ --firstname.lastname@example.org \ --cc=Johannes.Schindelin@gmx.de \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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).