Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

[Brandon Williams <bmwill@google.com>]

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams <bmwill@google.com> wrote:
> > The current pkt-line API encodes the status of a pkt-line read in the
> > length of the read content.  An error is indicated with '-1', a flush
> > with '0' (which can be confusing since a return value of '0' can also
> > indicate an empty pkt-line), and a positive integer for the length of
> > the read content otherwise.  This doesn't leave much room for allowing
> > the addition of additional special packets in the future.
> >
> > To solve this introduce 'packet_read_with_status()' which reads a packet
> > and returns the status of the read encoded as an 'enum packet_status'
> > type.  This allows for easily identifying between special and normal
> > packets as well as errors.  It also enables easily adding a new special
> > packet in the future.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  pkt-line.c | 55 ++++++++++++++++++++++++++++++++++++++++++-------------
> >  pkt-line.h | 15 +++++++++++++++
> >  2 files changed, 57 insertions(+), 13 deletions(-)
> >
> > diff --git a/pkt-line.c b/pkt-line.c
> > index 2827ca772..8d7cd389f 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
> >         return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
> >  }
> >
> > -int packet_read(int fd, char **src_buf, size_t *src_len,
> > -               char *buffer, unsigned size, int options)
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
> > +                                               char *buffer, unsigned size, int *pktlen,
> > +                                               int options)
> >  {
> > -       int len, ret;
> > +       int len;
> >         char linelen[4];
> >
> > -       ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> > -       if (ret < 0)
> > -               return ret;
> > +       if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> > +               return PACKET_READ_EOF;
> > +
> >         len = packet_length(linelen);
> >         if (len < 0)
> >                 die("protocol error: bad line length character: %.4s", linelen);
> > -       if (!len) {
> > +
> > +       if (len == 0) {
> >                 packet_trace("0000", 4, 0);
> > -               return 0;
> > +               return PACKET_READ_FLUSH;
> > +       } else if (len >= 1 && len <= 3) {
> > +               die("protocol error: bad line length character: %.4s", linelen);
> 
> I wonder how much libified code we want here already, maybe we could
> have PACKET_READ_ERROR as a return value here instead of die()ing.
> There could also be an option that tells this code to die on error, this reminds
> me of the repository discovery as well as the refs code, both of which have
> this pattern.
> 
> Currently this series is only upgrading commands that use the network
> anyway, so I guess die()ing in an ls-remote or fetch is no big deal,
> but it could
> be interesting to keep going once we have more of the partial clone
> stuff working
> (e.g. remote assisted log/blame would want to gracefully fall back instead of
> die()ing without any useful output, I would think.)

These are all things we could do, but the current code just dies and it
may be more hassle right now to change all the uses of packet_read to
handle errors gracefully.  But its definitely something we can do in the
future.

> 
> >         }
> > +
> >         len -= 4;
> > -       if (len >= size)
> > +       if ((len < 0) || ((unsigned)len >= size))
> >                 die("protocol error: bad line length %d", len);
> > -       ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> > -       if (ret < 0)
> > -               return ret;
> > +
> > +       if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
> > +               return PACKET_READ_EOF;
> >
> >         if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> >             len && buffer[len-1] == '\n')
> > @@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
> >
> >         buffer[len] = 0;
> >         packet_trace(buffer, len, 0);
> > -       return len;
> > +       *pktlen = len;
> > +       return PACKET_READ_NORMAL;
> > +}
> > +
> > +int packet_read(int fd, char **src_buffer, size_t *src_len,
> > +               char *buffer, unsigned size, int options)
> > +{
> > +       enum packet_read_status status;
> > +       int pktlen;
> > +
> > +       status = packet_read_with_status(fd, src_buffer, src_len,
> > +                                        buffer, size, &pktlen,
> > +                                        options);
> > +       switch (status) {
> > +       case PACKET_READ_EOF:
> > +               pktlen = -1;
> > +               break;
> > +       case PACKET_READ_NORMAL:
> > +               break;
> > +       case PACKET_READ_FLUSH:
> > +               pktlen = 0;
> > +               break;
> > +       }
> > +
> > +       return pktlen;
> >  }
> >
> >  static char *packet_read_line_generic(int fd,
> > diff --git a/pkt-line.h b/pkt-line.h
> > index 3dad583e2..06c468927 100644
> > --- a/pkt-line.h
> > +++ b/pkt-line.h
> > @@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
> >  int packet_read(int fd, char **src_buffer, size_t *src_len, char
> >                 *buffer, unsigned size, int options);
> >
> > +/*
> > + * Read a packetized line into a buffer like the 'packet_read()' function but
> > + * returns an 'enum packet_read_status' which indicates the status of the read.
> > + * The number of bytes read will be assigined to *pktlen if the status of the
> > + * read was 'PACKET_READ_NORMAL'.
> > + */
> > +enum packet_read_status {
> > +       PACKET_READ_EOF = -1,
> > +       PACKET_READ_NORMAL,
> > +       PACKET_READ_FLUSH,
> > +};
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, size_t *src_len,
> > +                                               char *buffer, unsigned size, int *pktlen,
> > +                                               int options);
> > +
> >  /*
> >   * Convenience wrapper for packet_read that is not gentle, and sets the
> >   * CHOMP_NEWLINE option. The return value is NULL for a flush packet,
> > --
> > 2.15.1.620.gb9897f4670-goog
> >

-- 
Brandon Williams