Jeff King <peff@xxxxxxxx> writes:
> On Thu, Sep 17, 2015 at 09:13:40AM -0700, Junio C Hamano wrote:
>
>> And your new caller that does O_NONBLOCK wants to do more than
>> looping upon EWOULDBLOCK. It certainly would not want us to loop
>> here.
>>
>> So I wonder if you can just O_NONBLOCK the fd and use the usual
>> strbuf_read(), i.e. without any change in this patch, and update
>> xread() to _unconditionally_ return when read(2) says EAGAIN or
>> EWOULDBLOCK.
>>
>> What would that break?
>
> Certainly anybody who does not realize their descriptor is O_NONBLOCK
> and is using the spinning for correctness. I tend to think that such
> sites are wrong, though, and would benefit from us realizing they are
> spinning.
With or without O_NONBLOCK, not looping around xread() _and_ relying
on the spinning for "correctness" means the caller is not getting
correctness anyway, I think, because xread() does return a short
read, and we deliberately and explicitly do so since 0b6806b9
(xread, xwrite: limit size of IO to 8MB, 2013-08-20).
> But I think you can't quite get away with leaving strbuf_read untouched
> in this case. On error, it wants to restore the original value of the
> strbuf before the strbuf_read call. Which means that we throw away
> anything read into the strbuf before we get EAGAIN, and the caller never
> gets to see it.
I agree we need to teach strbuf_read() that xread() is now nicer on
O_NONBLOCK; perhaps like this?
strbuf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index cce5eed..49104d7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -368,6 +368,8 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
if (cnt < 0) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK)
+ break;
if (oldalloc == 0)
strbuf_release(sb);
else
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html