Re: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Jacob Keller]

On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller  wrote:

Maybe change the title to "without blocking" instead of "nonblockingly"?

Regards,
Jake
--
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: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Jacob Keller]

On Mon, Sep 21, 2015 at 5:10 PM, Junio C Hamano  wrote:
>> This wrapper just restarts on EINTR, but not on EAGAIN like xread
>> does. This gives less guarantees on the actual reading rather than
>> on returning fast.
>
> The last sentence is a bit hard to parse, by the way.
>

I'd imagine something more like:

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.

Regards,
Jake
--
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: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Jacob Keller  writes:

> On Mon, Sep 21, 2015 at 3:39 PM, Stefan Beller  wrote:
>
> Maybe change the title to "without blocking" instead of "nonblockingly"?

Both suggestions make sense ;-)
--
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: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Stefan Beller  writes:

> This wrapper just restarts on EINTR, but not on EAGAIN like xread
> does. This gives less guarantees on the actual reading rather than
> on returning fast.
>
> Signed-off-by: Stefan Beller 
> ---
>  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 50267a4..54ce231 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -223,6 +223,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 even if the data is available.
> + */

This comment is somewhat misleading to readers who knew there was
xread() and this new one is a variant of it.  xread() does not say
anything about "if the data is available" (it just blocks and
insists reading that much if the data is not ready yet).  If we drop
the "even if ..." from the end, that confusion would be avoided.

Turning EWOULDBLOCK to EAGAIN is a very useful thing to help the
callers, and it deserves to be described in the above comment.

> +ssize_t xread_nonblock(int fd, void *buf, size_t len)
> +{
> + ssize_t nr;
> + if (len > MAX_IO_SIZE)
> + len = MAX_IO_SIZE;

This line is under-indented?

> + 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.
--
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: [PATCHv3 03/13] xread_nonblock: add functionality to read from fds nonblockingly

[Junio C Hamano]

Stefan Beller  writes:

> This wrapper just restarts on EINTR, but not on EAGAIN like xread
> does. This gives less guarantees on the actual reading rather than
> on returning fast.

The last sentence is a bit hard to parse, by the way.

It lets caller make some progress without stalling by marking the
file descriptor as nonblocking and calling this function, compared
to calling xread() which will not give control back until reading
the asked size (or EOF).

It is not like "reading in full is always the desireable thing to
do, and returning early is something that needs compromise on that
desirable behaviour to achieve", which was the way I read your
version.  These two kinds of callers want two different things, but
the phrase "less guarantees" conveys a misguided value judgement (in
that "wanting to read in full always trumps other considerations" is
the stance the person who is making the judgement makes) more than
useful information (e.g. "the caller may want the control back
without stalling, in which case this is a useful thing to use").

The implementation itself is exactly like we discussed, and looks
good.

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