Re: [WIP 02/15] pkt-line: introduce struct packet_reader

[Brandon Williams <bmwill@google.com>]

On 12/07, Stefan Beller wrote:
> On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams <bmwill@google.com> wrote:
> > Sometimes it is advantageous to be able to peek the next packet line
> > without consuming it (e.g. to be able to determine the protocol version
> > a server is speaking).  In order to do that introduce 'struct
> > packet_reader' which is an abstraction around the normal packet reading
> > logic.  This enables a caller to be able to peek a single line at a time
> > using 'packet_reader_peek()' and having a caller consume a line by
> > calling 'packet_reader_read()'.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  pkt-line.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  pkt-line.h | 20 ++++++++++++++++++++
> >  2 files changed, 81 insertions(+)
> >
> > diff --git a/pkt-line.c b/pkt-line.c
> > index ac619f05b..518109bbe 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -406,3 +406,64 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out)
> >         }
> >         return sb_out->len - orig_len;
> >  }
> > +
> > +/* Packet Reader Functions */
> > +void packet_reader_init(struct packet_reader *reader, int fd,
> > +                       char *src_buffer, size_t src_len)
> > +{
> > +       reader->fd = fd;
> > +       reader->src_buffer = src_buffer;
> > +       reader->src_len = src_len;
> > +       reader->buffer = packet_buffer;
> > +       reader->buffer_size = sizeof(packet_buffer);
> > +       reader->options = PACKET_READ_CHOMP_NEWLINE | PACKET_READ_GENTLE_ON_EOF;
> 
> I assume the future user of this packet reader will need exactly
> these options coincidentally. ;)
> 
> I think it might be ok for now and later we can extend the reader if needed
> to also take the flags as args. However given this set of args, this is a gentle
> packet reader, as it corresponds to the _gently version of reading
> packets AFAICT.
> 
> Unlike the pkt_read function this constructor of a packet reader doesn't need
> the arguments for its buffer (packet_buffer and sizeof thereof), which
> packet_read
> unfortunately needs. We pass in packet_buffer all the time except in
> builtin/receive-pack
> for obtaining the gpg cert. I think that's ok here.

Yeah, all of the callers I wanted to migrate all passed the same flags
and same buffer.  I figured there may be a point in the future where
we'd want to extend this so instead of hard coding the flags in the read
functions, I did so in the constructor.  That way if we wanted to extend
it in the future, it would be a little bit easier.

> 
> > +enum packet_read_status packet_reader_read(struct packet_reader *reader)
> > +{
> > +       if (reader->line_peeked) {
> > +               reader->line_peeked = 0;
> > +               return reader->status;
> > +       }
> > +
> > +       reader->status = packet_read_with_status(reader->fd,
> > +                                                &reader->src_buffer,
> > +                                                &reader->src_len,
> > +                                                reader->buffer,
> > +                                                reader->buffer_size,
> > +                                                &reader->pktlen,
> > +                                                reader->options);
> > +
> > +       switch (reader->status) {
> > +       case PACKET_READ_ERROR:
> > +               reader->pktlen = -1;
> 
> In case of error the read function might not
> have assigned the pktlen as requested, so we assign
> it to -1/NULL here. Though the caller ought to already know
> that they handle bogus, as the state is surely the first thing
> they'd inspect?

Exactly, I thought it would be easier to ensure that pktlen and line
were had consistent state no matter what the output of the read was.
But yes, callers should be looking at the status to determine what to
do.

> 
> > +               reader->line = NULL;
> > +               break;
> > +       case PACKET_READ_NORMAL:
> > +               reader->line = reader->buffer;
> > +               break;
> > +       case PACKET_READ_FLUSH:
> > +               reader->pktlen = 0;
> > +               reader->line = NULL;
> > +               break;
> > +       }
> 
> Oh, this gives an interesting interface for someone who is
> just inspecting the len/line instead of the state, so it might be
> worth keeping it this way.
> 
> Can we have an API documentation in the header file,
> explaining what to expect in each field given the state
> of the (read, peaked) packet?

Yep I can write some better API docs for these functions.

-- 
Brandon Williams