Re: [PATCH v2 1/6] compat: add function to enable nonblocking pipes

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2022, 20:23 UTC
Message-ID: <xmqqwnb6ty9u.fsf@gitster.g>
In-Reply-To: <YvyFBzdO8PN7Ou0W@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

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