On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams <bmw...@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 <bmw...@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
