Re: [WIP 02/15] pkt-line: introduce struct packet_reader
- From
Stefan Beller <sbeller@google.com>
- Date
- Dec 7, 2017, 22:01 UTC
- Message-ID
- <CAGZ79kY5dCo1qO-4W=xdZQMjoJs1ZqJdywUsqX0CjyXHu80hCw@mail.gmail.com>
- In-Reply-To
- <20171204235823.63299-3-bmwill@google.com>
On Mon, Dec 4, 2017 at 3:58 PM, Brandon Williams <bmwill@google.com> wrote:
Show 33 quoted lines
> 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.
Show 18 quoted lines
> +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?
Show 10 quoted lines
> + 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?