[PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Provide a wrapper to read(), similar to xread(), that restarts on
EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
handle polling itself, possibly polling multiple sockets or performing
some other action.
Helped-by: Jacob Keller
Helped-by: Jeff King ,
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
git-compat-util.h | 1 +
wrapper.c | 22 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index c6d391f..9ccea85 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -718,6 +718,7 @@ extern void *xcalloc(size_t nmemb, size_t size);
extern void *xmmap(void *start, size_t length, int prot, int flags, int fd,
off_t offset);
extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int
fd, off_t offset);
extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
extern ssize_t xwrite(int fd, const void *buf, size_t len);
extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
extern int xdup(int fd);
diff --git a/wrapper.c b/wrapper.c
index 5517928..41a21e1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -217,6 +217,28 @@ ssize_t xread(int fd, void *buf, size_t len)
}
/*
+ * xread_nonblock() is the same a read(), but it automatically restarts read()
+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
+ * "len" bytes is read. EWOULDBLOCK is turned into EAGAIN.
+ */
+ssize_t xread_nonblock(int fd, void *buf, size_t len)
+{
+ ssize_t nr;
+ if (len > MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
+ while (1) {
+ nr = read(fd, buf, len);
+ if (nr < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ errno = EAGAIN;
+ }
+ return nr;
+ }
+}
+
+/*
* xwrite() is the same a write(), but it automatically restarts write()
* operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
* GUARANTEE that "len" bytes is written even if the operation is successful.
--
2.5.0.273.g6fa2560.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Provide a wrapper to read(), similar to xread(), that restarts on
EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
handle polling itself, possibly polling multiple sockets or performing
some other action.
Helped-by: Jacob Keller
Helped-by: Jeff King ,
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
git-compat-util.h | 1 +
wrapper.c | 22 ++
2 files changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 8e39867..87456a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -723,6 +723,7 @@ extern void *xmmap(void *start, size_t length, int prot,
int flags, int fd, off_
extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int
fd, off_t offset);
extern int xopen(const char *path, int flags, ...);
extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
extern ssize_t xwrite(int fd, const void *buf, size_t len);
extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
extern int xdup(int fd);
diff --git a/wrapper.c b/wrapper.c
index 4f720fe..f71237c 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -252,6 +252,28 @@ ssize_t xread(int fd, void *buf, size_t len)
}
/*
+ * xread_nonblock() is the same a read(), but it automatically restarts read()
+ * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
+ * "len" bytes is read. EWOULDBLOCK is turned into EAGAIN.
+ */
+ssize_t xread_nonblock(int fd, void *buf, size_t len)
+{
+ ssize_t nr;
+ if (len > MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
+ while (1) {
+ nr = read(fd, buf, len);
+ if (nr < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ errno = EAGAIN;
+ }
+ return nr;
+ }
+}
+
+/*
* xwrite() is the same a write(), but it automatically restarts write()
* operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
* GUARANTEE that "len" bytes is written even if the operation is successful.
--
2.6.4.443.ge094245.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Stefan Beller writes: > Provide a wrapper to read(), similar to xread(), that restarts on > EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to > handle polling itself, possibly polling multiple sockets or performing > some other action. Do you still need this (and use of this in 4/8)? The description of a4433fd4 (run-command: remove set_nonblocking(), 2015-11-05) from the previous iteration justifies the removal of set_nonblocking() this way: run-command: remove set_nonblocking() strbuf_read_once can also operate on blocking file descriptors if we are sure they are ready. And the poll(2) we call before calling this ensures that this is the case. Updated run-command in this reroll lacks set_nonblocking(), and does have the poll(2) before it calls strbuf_read_once(). Which means that xread_nonblock() introduced here and used by [4/8] would read a file descriptor that is not marked as nonblock, the read(2) in there would never error-return with EWOULDBLOCK, and would be identical to xread() updated by [2/8] in this reroll. So it appears to me that you can lose this and call xread() in [4/8]? Ahh, there is a difference if the file descriptor the caller feeds strbuf_read_once() happens to be marked as nonblock. read_once() wants to return without doing the poll() in such a case. Even though this series would not introduce any use of a nonblocking file descriptor, as a general API function, [4/8] must be prepared for such a future caller, hence [3/8] is needed. OK, thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 2:37 PM, Stefan Beller wrote:
> Provide a wrapper to read(), similar to xread(), that restarts on
> EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
> handle polling itself, possibly polling multiple sockets or performing
> some other action.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/wrapper.c b/wrapper.c
> @@ -252,6 +252,28 @@ ssize_t xread(int fd, void *buf, size_t len)
> + * xread_nonblock() is the same a read(), but it automatically restarts
> read()
> + * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that
> + * "len" bytes is read. EWOULDBLOCK is turned into EAGAIN.
The last sentence is confusing. From the commit message, we learn that
this function doesn't care about EAGAIN or EWOULDBLOCK, yet the above
comment seems to imply that it does. What it really ought to be saying
is that "as a convenience, errno is transformed from EWOULDBLOCK to
EAGAIN so that the caller only has to check for EAGAIN".
(It also feels weird that this should be messing with errno at all,
but that's a different issue...)
> + */
> +ssize_t xread_nonblock(int fd, void *buf, size_t len)
> +{
> + ssize_t nr;
> + if (len > MAX_IO_SIZE)
> + len = MAX_IO_SIZE;
> + while (1) {
> + nr = read(fd, buf, len);
> + if (nr < 0) {
> + if (errno == EINTR)
> + continue;
> + if (errno == EWOULDBLOCK)
> + errno = EAGAIN;
> + }
> + return nr;
> + }
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 6:03 PM, Eric Sunshine wrote: > On Mon, Dec 14, 2015 at 2:37 PM, Stefan Beller wrote: >> Provide a wrapper to read(), similar to xread(), that restarts on >> EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to >> handle polling itself, possibly polling multiple sockets or performing >> some other action. >> >> Signed-off-by: Stefan Beller >> --- >> diff --git a/wrapper.c b/wrapper.c >> @@ -252,6 +252,28 @@ ssize_t xread(int fd, void *buf, size_t len) >> + * xread_nonblock() is the same a read(), but it automatically restarts >> read() >> + * interrupted operations (EINTR). xread_nonblock() DOES NOT GUARANTEE that >> + * "len" bytes is read. EWOULDBLOCK is turned into EAGAIN. > > The last sentence is confusing. From the commit message, we learn that > this function doesn't care about EAGAIN or EWOULDBLOCK, yet the above > comment seems to imply that it does. What it really ought to be saying > is that "as a convenience, errno is transformed from EWOULDBLOCK to > EAGAIN so that the caller only has to check for EAGAIN". I forgot to mention that the mutation of EWOULDBLOCK into EAGAIN is something that should be described in the header rather than here at the source. In fact, the entire comment block above this function is more suitable in the header file. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Eric Sunshine writes:
> The last sentence is confusing. From the commit message, we learn that
> this function doesn't care about EAGAIN or EWOULDBLOCK, yet the above
> comment seems to imply that it does. What it really ought to be saying
> is that "as a convenience, errno is transformed from EWOULDBLOCK to
> EAGAIN so that the caller only has to check for EAGAIN".
Let's do this for now, then.
-- >8 --
From: Stefan Beller
Date: Mon, 14 Dec 2015 11:37:13 -0800
Subject: [PATCH] xread_nonblock: add functionality to read from fds without
blocking
Provide a wrapper to read(), similar to xread(), that restarts on
EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to
handle polling itself, possibly polling multiple sockets or performing
some other action.
Helped-by: Jacob Keller
Helped-by: Jeff King ,
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
Acked-by: Johannes Sixt
Signed-off-by: Junio C Hamano
---
git-compat-util.h | 1 +
wrapper.c | 24
2 files changed, 25 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 8e39867..87456a3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -723,6 +723,7 @@ extern void *xmmap(void *start, size_t length, int prot,
int flags, int fd, off_
extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int
fd, off_t offset);
extern int xopen(const char *path, int flags, ...);
extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xread_nonblock(int fd, void *buf, size_t len);
extern ssize_t xwrite(int fd, const void *buf, size_t len);
extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
extern int xdup(int fd);
diff --git a/wrapper.c b/wrapper.c
index 1770efa..0b5b03d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -259,6 +259,30 @@ ssize_t xread(int fd, void *buf, size_t len)
}
/*
+ * xread_nonblock() automatically restarts interrupted operations
+ * (EINTR). xread_nonblock() DOES NOT GUARANTEE that "len" bytes is
+ * read. For convenience to callers that mark the file descriptor
+ * non-blocking, EWOULDBLOCK is turned into EAGAIN to allow them to
+ * check only for EAGAIN (POSIX.1 allows either to be returned).
+ */
+ssize_t xread_nonblock(int fd, void *buf, size_t len)
+{
+ ssize_t nr;
+ if (len > MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
+ while (1) {
+ nr = read(fd, buf, len);
+ if (nr < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno == EWOULDBLOCK)
+ errno = EAGAIN;
+ }
+ return nr;
+ }
+}
+
+/*
* xwrite() is the same a write(), but it automatically restarts write()
* operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
* GUARANTEE that "len" bytes is written even if the operation is successful.
--
2.7.0-rc0-109-gb762328
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 03:15:29PM -0800, Junio C Hamano wrote: > -- >8 -- > From: Stefan Beller > Date: Mon, 14 Dec 2015 11:37:13 -0800 > Subject: [PATCH] xread_nonblock: add functionality to read from fds without > blocking > > Provide a wrapper to read(), similar to xread(), that restarts on > EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to > handle polling itself, possibly polling multiple sockets or performing > some other action. This makes me wonder why we restart xread() on EAGAIN in the first place. On EINTR, sure; signals can come and we want to keep going. But if do not have non-blocking descriptors, it should never happen, right? Are we trying to protect ourselves against somebody _else_ giving us a non-blocking descriptor? In that case we'll quietly spin and waste CPU. Which isn't great, but perhaps better than returning an error. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 3:57 PM, Jeff King wrote: > On Mon, Dec 14, 2015 at 03:15:29PM -0800, Junio C Hamano wrote: > >> -- >8 -- >> From: Stefan Beller >> Date: Mon, 14 Dec 2015 11:37:13 -0800 >> Subject: [PATCH] xread_nonblock: add functionality to read from fds without >> blocking >> >> Provide a wrapper to read(), similar to xread(), that restarts on >> EINTR but not EAGAIN (or EWOULDBLOCK). This enables the caller to >> handle polling itself, possibly polling multiple sockets or performing >> some other action. > > This makes me wonder why we restart xread() on EAGAIN in the first > place. > > On EINTR, sure; signals can come and we want to keep going. But if do > not have non-blocking descriptors, it should never happen, right? right. > > Are we trying to protect ourselves against somebody _else_ giving us a > non-blocking descriptor? In that case we'll quietly spin and waste CPU. > Which isn't great, but perhaps better than returning an error. Yes. This sounds like a good reasoning for 2/8 (add in the poll, so we are more polite), though. This patch is a prerequisite for 4/8, which explicitly doesn't want to loop but a quick return. Maybe we could even drop this patch and just use `read` as is in 4/8. Looking from a higher level perspective, we don't care about strbuf_read_nonblocking to return after a signal without retry. > > -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 04:09:01PM -0800, Stefan Beller wrote: > > Are we trying to protect ourselves against somebody _else_ giving us a > > non-blocking descriptor? In that case we'll quietly spin and waste CPU. > > Which isn't great, but perhaps better than returning an error. > > Yes. > This sounds like a good reasoning for 2/8 (add in the poll, so we are > more polite), though. > > This patch is a prerequisite for 4/8, which explicitly doesn't want to loop > but a quick return. Maybe we could even drop this patch and just use > `read` as is in 4/8. Looking from a higher level perspective, we don't care > about strbuf_read_nonblocking to return after a signal without retry. I was actually thinking about simply teaching xread() not to worry about EAGAIN, but that would probably be a regression in the "whoops, somebody gave us a non-blocking stdin!" case. But yeah, I think simply using xread() as-is in strbuf_read_once (or whatever it ends up being called) is OK. I think all of the _intentionally_ non-blocking descriptors are gone in this iteration, right? So the caller of strbuf_read_once expects to have to call poll() or to block. And that's what xread() does. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 4:16 PM, Jeff King wrote:
> On Mon, Dec 14, 2015 at 04:09:01PM -0800, Stefan Beller wrote:
>
>> > Are we trying to protect ourselves against somebody _else_ giving us a
>> > non-blocking descriptor? In that case we'll quietly spin and waste CPU.
>> > Which isn't great, but perhaps better than returning an error.
>>
>> Yes.
>> This sounds like a good reasoning for 2/8 (add in the poll, so we are
>> more polite), though.
>>
>> This patch is a prerequisite for 4/8, which explicitly doesn't want to loop
>> but a quick return. Maybe we could even drop this patch and just use
>> `read` as is in 4/8. Looking from a higher level perspective, we don't care
>> about strbuf_read_nonblocking to return after a signal without retry.
>
> I was actually thinking about simply teaching xread() not to worry about
> EAGAIN, but that would probably be a regression in the "whoops, somebody
> gave us a non-blocking stdin!" case.
>
> But yeah, I think simply using xread() as-is in strbuf_read_once (or
> whatever it ends up being called) is OK.
I was actually thinking about using {without-x}read, just the plain system call.
Do we have any issues with that for wrapping purposes for Windows?
There is no technical reason to prefer xread over read in strbuf_read_once as
* we are not nonblocking (so the EAGAIN|| EWOULDBLOCK doesn't apply)
* we don't care about EINTR and retrying upon that signal
* we would not care about MAX_IO_SIZE most likely (that's actually one
of the reasons I could technically think of to prefer xread)
> I think all of the
> _intentionally_ non-blocking descriptors are gone in this iteration,
> right?
I think we don't even have unintentional non blocking fds here now as we
create all the fds ourselves and never set the NOBLOCK flag.
> So the caller of strbuf_read_once expects to have to call poll()
> or to block. And that's what xread() does.
ok, I'll drop this patch and use xread there.
>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Jeff King writes: > Are we trying to protect ourselves against somebody _else_ giving us a > non-blocking descriptor? In that case we'll quietly spin and waste CPU. > Which isn't great, but perhaps better than returning an error. I think I said it earlier in a message upthread. > Ahh, there is a difference if the file descriptor the caller feeds > strbuf_read_once() happens to be marked as nonblock. read_once() > wants to return without doing the poll() in such a case. Even > though this series would not introduce any use of a nonblocking file > descriptor, as a general API function, [4/8] must be prepared for > such a future caller, hence [3/8] is needed. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
On Mon, Dec 14, 2015 at 04:25:18PM -0800, Stefan Beller wrote:
> > But yeah, I think simply using xread() as-is in strbuf_read_once (or
> > whatever it ends up being called) is OK.
>
> I was actually thinking about using {without-x}read, just the plain system
> call.
> Do we have any issues with that for wrapping purposes for Windows?
> There is no technical reason to prefer xread over read in strbuf_read_once as
> * we are not nonblocking (so the EAGAIN|| EWOULDBLOCK doesn't apply)
> * we don't care about EINTR and retrying upon that signal
> * we would not care about MAX_IO_SIZE most likely (that's actually one
> of the reasons I could technically think of to prefer xread)
I think you do still need to care about EINTR, or at least not barfing
if read() returns -1. If I understand correctly, you want to do
something like:
while (1) {
poll(some_fds);
for (i = 0; i < nr_fds; i++) {
if (some_fds[i].revents & POLLIN) {
int r = strbuf_read_once(buf[i], some_fds[i]);
/* ??? what do we do with r? */
}
}
}
If we get EINTR from that read, it's OK for us to loop back to the
poll() and go again.
But if we get a true error in "r", we'd want to know, right? That means
we must distinguish between EINTR and "real" errors (like EIO or
something). We can do that here, but I think it's just as easy to do it
inside of strbuf_read_once (by calling xread() there). It's OK not to
jump back to the poll(), because we know the data that triggered the
POLLIN is still waiting for us to read it.
And we are fine with EAGAIN, too. We don't expect the sockets to be
non-blocking in the first place, but even if they were, we know we just
got POLLIN, so there should be data waiting.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking
Am 15.12.2015 um 01:25 schrieb Stefan Beller:
I was actually thinking about using {without-x}read, just the plain system call.
Do we have any issues with that for wrapping purposes for Windows?
xread() limits the size being read to MAX_IO_SIZE, which is needed on
some systems (I think that some Windows configurations did fall into
this category).
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
