Re: [RFC PATCH] fetch-pack: try harder to read an ERR packet

[Christian Couder <christian.couder@gmail.com>]

On Thu, Apr 23, 2020 at 1:27 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Apr 22, 2020 at 06:33:57PM +0200, Christian Couder wrote:
>
> It looks like this may be missing a:
>
>   From: SZEDER Gábor <szeder.dev@gmail.com>
>
> header.

Yeah, I wanted to have him as the author but forgot. The next version
will have it.

[...]

> > ---
> > This just formats the following patch from SZEDER Gábor:
> >
> > https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/
> >
> > I haven't tried to implement some suggestions discussed later
> > in the above thread like:
> >
> >   - renaming send_request()
>
> Agreed that this is probably something we should do. Maybe
> 'send_request_retry' or something? That name is kind of terrible.

Not sure 'send_request_retry' is better as we are not really retrying
to send the request. My take would be something like
'send_request_read_err'. For now I have left it as is though.

> >   - covering more code pathes
>
> I see where Peff raised this point originally, but I think that this is
> a good step forward as-is. No need to hold this up looking for complete
> coverage, since this is obviously improving the situation.

Ok.

> >   - avoid blocking indefinitely by looking for an ERR packet
> >     only if the write() resulted in ECONNRESET or EPIPE
>
> I think that I may have addressed this point below.
>
> >   - first printing an error message with error_errno() before
> >     going on to look for an ERR packet
>
> I'm not sure what I think about this one. I'd certainly welcome all
> opinions on this matter.
>
> >   - implementing a timeout
>
> A timeout may be a good thing to do. See what you think about my
> suggestion below, first, though.

Ok, thanks for your suggestion.

> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 1734a573b0..63e8925e2b 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -185,14 +185,27 @@ static enum ack_type get_ack(struct packet_reader *reader,
> >  }
> >
> >  static void send_request(struct fetch_pack_args *args,
> > -                      int fd, struct strbuf *buf)
> > +                      int fd, struct strbuf *buf,
> > +                      struct packet_reader *reader)
> >  {
> >       if (args->stateless_rpc) {
> >               send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
> >               packet_flush(fd);
> >       } else {
> > -             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.

Yeah, good idea.

> 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.

Yeah, your code above looks correct. I have added a new function doing
the above in the new version I will send soon.

> This way, we only read exactly as many packets as we need to when we hit
> this case. I'm not sure that it matters in practice, though.

I am not sure I understand what you think doesn't matter in practice.

Thanks,
Christian.