| Message ID | YvyFBzdO8PN7Ou0W@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 10f743389ca9a92720fb9c3d15f647888d82c297 |
| Headers | show |
| Series | fix pipe_command() deadlock | expand |
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.
On Wed, Aug 17, 2022 at 01:23:25PM -0700, Junio C Hamano wrote: > 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. My goal was to explain that "huh" with this bit in the commit message: > > 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). without going into too many details. I do think it would be easier to explain if the Windows implementation was added in the same commit (since it is explicitly just about pipes), but I wanted to credit René as the author there. I'm not sure if it's worth folding them together and adding a co-author credit instead. Or maybe I should state outright in this commit message that we're doing it for Windows. ;) -Peff
diff --git a/Makefile b/Makefile index e8adeb09f1..224e193b66 100644 --- a/Makefile +++ b/Makefile @@ -918,6 +918,7 @@ LIB_OBJS += combine-diff.o LIB_OBJS += commit-graph.o LIB_OBJS += commit-reach.o LIB_OBJS += commit.o +LIB_OBJS += compat/nonblock.o LIB_OBJS += compat/obstack.o LIB_OBJS += compat/terminal.o LIB_OBJS += compat/zlib-uncompress2.o diff --git a/compat/nonblock.c b/compat/nonblock.c new file mode 100644 index 0000000000..b08105a21d --- /dev/null +++ b/compat/nonblock.c @@ -0,0 +1,23 @@ +#include "git-compat-util.h" +#include "nonblock.h" + +#ifdef O_NONBLOCK + +int enable_pipe_nonblock(int fd) +{ + int flags = fcntl(fd, F_GETFL); + if (flags < 0) + return -1; + flags |= O_NONBLOCK; + return fcntl(fd, F_SETFL, flags); +} + +#else + +int enable_pipe_nonblock(int fd) +{ + errno = ENOSYS; + return -1; +} + +#endif diff --git a/compat/nonblock.h b/compat/nonblock.h new file mode 100644 index 0000000000..af1a331301 --- /dev/null +++ b/compat/nonblock.h @@ -0,0 +1,9 @@ +#ifndef COMPAT_NONBLOCK_H +#define COMPAT_NONBLOCK_H + +/* + * Enable non-blocking I/O for the pipe specified by the passed-in descriptor. + */ +int enable_pipe_nonblock(int fd); + +#endif
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. Signed-off-by: Jeff King <peff@peff.net> --- Makefile | 1 + compat/nonblock.c | 23 +++++++++++++++++++++++ compat/nonblock.h | 9 +++++++++ 3 files changed, 33 insertions(+) create mode 100644 compat/nonblock.c create mode 100644 compat/nonblock.h