git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "René Scharfe" <l.s.r@web.de>,
	git@vger.kernel.org,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes
Date: Wed, 17 Aug 2022 13:23:25 -0700	[thread overview]
Message-ID: <xmqqwnb6ty9u.fsf@gitster.g> (raw)
In-Reply-To: YvyFBzdO8PN7Ou0W@coredump.intra.peff.net

Jeff King <peff@peff.net> writes:

> We'd like to be able to make some of our pipes nonblocking so that
> poll() can be used effectively, but O_NONBLOCK isn't portable. Let's
> introduce a compat wrapper so this can be abstracted for each platform.
>
> The interface is as narrow as possible to let platforms do what's
> natural there (rather than having to implement fcntl() and a fake
> O_NONBLOCK for example, or having to handle other types of descriptors).
>
> The next commit will add Windows support, at which point we should be
> covering all platforms in practice. But if we do find some other
> platform without O_NONBLOCK, we'll return ENOSYS. Arguably we could just
> trigger a build-time #error in this case, which would catch the problem
> earlier. But since we're not planning to use this compat wrapper in many
> code paths, a seldom-seen runtime error may be friendlier for such a
> platform than blocking compilation completely. Our test suite would
> still notice it.

Very well reasoned.

The only small "huh?" factor was that the name of the helper is not
enable_nonblock(), but singles out pipe as valid thing to work on.
I think it is perfectly fine, given that the current plan we have is
to use this on the pipe with the command we spawn with
pipe_command(), and it probably is even preferrable to explicitly
declare that this is designed to only work with pipes and future
developers who try to abuse it on other kinds of file descriptors
are on their own.  And "pipe" in the name of this helper may be such
a declaration that is strong enough.

Thanks.

  reply	other threads:[~2022-08-17 20:24 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-02  4:13 [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-02 15:04 ` Junio C Hamano
2022-08-02 15:39 ` Jeff King
2022-08-02 16:16   ` Junio C Hamano
2022-08-03  3:53     ` [PATCH v2] " Jeff King
2022-08-03 16:45       ` René Scharfe
2022-08-03 17:20         ` Jeff King
2022-08-03 21:56           ` René Scharfe
2022-08-05 15:36             ` Jeff King
2022-08-05 21:13               ` René Scharfe
2022-08-07 10:15                 ` René Scharfe
2022-08-07 17:41                   ` Jeff King
2022-08-10  5:39                     ` René Scharfe
2022-08-10 19:53                       ` Jeff King
2022-08-10 22:35                         ` René Scharfe
2022-08-11  8:52                           ` Jeff King
2022-08-10  5:39                     ` [PATCH] mingw: handle writes to non-blocking pipe René Scharfe
2022-08-10  9:07                       ` Johannes Schindelin
2022-08-10 20:02                       ` Jeff King
2022-08-10 22:34                         ` René Scharfe
2022-08-11  8:47                           ` Jeff King
2022-08-11 17:35                             ` René Scharfe
2022-08-11 18:20                               ` Jeff King
2022-08-14 15:37                                 ` René Scharfe
2022-08-17  5:39                                   ` Jeff King
2022-08-17  6:04                                     ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-17  6:04                                       ` [PATCH v2 1/6] compat: add function to enable nonblocking pipes Jeff King
2022-08-17 20:23                                         ` Junio C Hamano [this message]
2022-08-18  5:41                                           ` Jeff King
2022-08-17  6:05                                       ` [PATCH v2 2/6] nonblock: support Windows Jeff King
2022-08-17  6:06                                       ` [PATCH v2 3/6] git-compat-util: make MAX_IO_SIZE define globally available Jeff King
2022-08-17  6:08                                       ` [PATCH v2 4/6] pipe_command(): avoid xwrite() for writing to pipe Jeff King
2022-08-17  6:09                                       ` [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe Jeff King
2022-08-17 18:57                                         ` Junio C Hamano
2022-08-18  5:38                                           ` Jeff King
2022-08-17  6:10                                       ` [PATCH v2 6/6] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-17  6:20                                       ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-19 21:19                                       ` René Scharfe
2022-08-20  7:04                                         ` Jeff King
2022-08-07 10:14               ` [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking René Scharfe
2022-08-08 12:55                 ` Johannes Schindelin
2022-08-08 12:59       ` Johannes Schindelin
2022-08-09 13:04         ` Jeff King
2022-08-09 22:10           ` Johannes Schindelin

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=xmqqwnb6ty9u.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=peff@peff.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).