On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams <bmwill@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
> +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?
> + 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?
