On Thu, Sep 17, 2015 at 10:54:39AM -0700, Junio C Hamano wrote:
> OK. Trying to repurpose strbuf_read() for non-blocking FD was a
> silly idea to begin with, as it wants to read thru to the EOF, and
> setting FD explicitly to O_NONBLOCK is a sign that the caller wants
> to grab as much as possible and does not want to wait for the EOF.
>
> So assuming we want strbuf_read_nonblock(), what interface do we
> want from it? We could:
>
> * Have it grab as much as possible into sb as long as it does not
> block?
>
> * Have it grab reasonably large amount into sb, and not blocking is
> an absolute requirement, but the function is not required to read
> everything that is available on the FD (i.e. the caller is
> expected to loop)?
I think we are crossing emails, but I would definitely argue for the
latter.
> If we choose the latter, then your "EAGAIN? EOF?" becomes easier,
> no? We only have to do a single call to xread(), and then we:
>
> - get EAGAIN or EWOULDBLOCK; leave sb as-is, set errno==EAGAIN and
> return -1.
>
> - get something (in which case that is not an EOF yet); append to
> sb, return the number of bytes.
>
> - get EOF; leave sb as-is, return 0.
Yes, exactly.
> ssize_t strbuf_read_nonblock(struct strbuf *sb, int fd, size_t hint)
> {
> strbuf_grow(sb, hint ? hint : 8192);
> ssize_t want = sb->alloc - sb->len - 1;
> ssize_t got = xread_nonblock(fd, sb->buf + sb->len, want);
> if (got < 0) {
> if (errno == EWOULDBLOCK)
> errno = EAGAIN; /* make life easier for the caller */
> return got;
> }
I like the normalizing of errno, but that should probably be part of
xread_nonblock(), I would think.
> sb->len += got;
> sb->buf[sb->len] = '\0';
Use strbuf_setlen() here?
> return got;
If "got == 0", we naturally do not change sb->len at all, and the strbuf
is left unchanged. But do we want to de-allocate what we allocated in
strbuf_grow() above? That is what strbuf_read() does, but I think it is
even less likely to help anybody here. With the original strbuf_read(),
you might do:
if (strbuf_read(&buf, fd, hint) <= 0)
return; /* got nothing */
but because the nature of strbuf_read_nonblock() is to call it from a
loop, you'd want to strbuf_release() when you leave the loop anyway.
-Peff
--
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