Re: [RFC PATCH] fetch-pack: try harder to read an ERR packet
- From
Jeff King <peff@peff.net>
- Date
- Apr 28, 2020, 08:33 UTC
- Message-ID
- <20200428083336.GA2405176@coredump.intra.peff.net>
- In-Reply-To
- <20200422232744.GA19100@syl.local>
On Wed, Apr 22, 2020 at 05:27:44PM -0600, Taylor Blau wrote:
Show 34 quoted lines
> > - if (write_in_full(fd, buf->buf, buf->len) < 0)
> > + if (write_in_full(fd, buf->buf, buf->len) < 0) {
>
> This makes sense; if 'write_in_full' fails, we want to go into the
> error-handling case below.
>
> But, I wonder if we could do better than re-using 'write_in_full' here.
> We definitely do want to write 'buf->len' bytes overall, but what
> happens when a 'write()' fails? I think it's at _that_ point we want to
> try and read a packet or two off from the remote.
>
> What if instead this looked something like:
>
> const char *p = buf;
> ssize_t total = 0;
>
> while (count > 0) {
> ssize_t written = xwrite(fd, p, count);
> if (written < 0)
> return -1;
> /* note the change on this line */
> if (!written && packet_reader_read(reader) == PACKET_READ_EOF) {
> errno = ENOSPC;
> return -1;
> }
> count -= written;
> p += written;
> total += written;
> }
>
> return total;
>
> That is basically the definition of 'write_in_full', but when we didn't
> get a chance to write anything, then we try to read one packet.I'm not sure I understand what this is trying to do. If write_in_full() returns an error, then we know that write() just failed, and it would be appropriate to check errno at that point and decide whether to read a packet.
The code you've written above doesn't make sense to me. If we see an error, we'd return _before_ doing any packet_reader_read() stuff. We'd trigger it only when write() returns 0. But it should only do that if we fed it 0 bytes, which we know we'd never pass (because we wouldn't run the loop if count==0).
-Peff