Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Jonathan Nieder]

Brandon Williams wrote:
> On 02/12, Jonathan Nieder wrote:

>>> --- a/pkt-line.h
>>> +++ b/pkt-line.h
>>> @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t 
>>> *src_len, int *size);
>>>   */
>>>  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>>>  
>>> +struct packet_reader {
>>> +   /* source file descriptor */
>>> +   int fd;
>>> +
>>> +   /* source buffer and its size */
>>> +   char *src_buffer;
>>> +   size_t src_len;
>>
>> Can or should this be a strbuf?
>>
>>> +
>>> +   /* buffer that pkt-lines are read into and its size */
>>> +   char *buffer;
>>> +   unsigned buffer_size;
>>
>> Likewise.
>
> This struct is setup to be a drop in replacement for the existing
> read_packet() family of functions.  Because of this I tried to make the
> interface as similar as possible to make it easy to convert to using it
> as well as having no need to clean anything up (because the struct is
> really just a wrapper and doesn't own anything).

Sorry, I don't completely follow.  Are you saying some callers play
with the buffer, or are you saying you haven't checked?  (If the
latter, that's perfectly fine; I'm just trying to understand the API.)

Either way, can you add some comments about ownership / who is allowed
to write to it / etc to make it easier to clean up later?

Thanks,
Jonathan

Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Brandon Williams]

On 02/12, Jonathan Nieder wrote:
> [...]
> > --- a/pkt-line.h
> > +++ b/pkt-line.h
> > @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t 
> > *src_len, int *size);
> >   */
> >  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
> >  
> > +struct packet_reader {
> > +   /* source file descriptor */
> > +   int fd;
> > +
> > +   /* source buffer and its size */
> > +   char *src_buffer;
> > +   size_t src_len;
> 
> Can or should this be a strbuf?
> 
> > +
> > +   /* buffer that pkt-lines are read into and its size */
> > +   char *buffer;
> > +   unsigned buffer_size;
> 
> Likewise.
> 

This struct is setup to be a drop in replacement for the existing
read_packet() family of functions.  Because of this I tried to make the
interface as similar as possible to make it easy to convert to using it
as well as having no need to clean anything up (because the struct is
really just a wrapper and doesn't own anything).

-- 
Brandon Williams

Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Jonathan Nieder]

Jonathan Nieder wrote:
> Brandon Williams 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 
>> ---
>>  pkt-line.c | 59 +++
>>  pkt-line.h | 58 ++
>>  2 files changed, 117 insertions(+)
>
> I like it!
>
> The questions and nits from
> https://public-inbox.org/git/20180213004937.gb42...@aiede.svl.corp.google.com/
> still apply.  In particular, the ownership of the buffers inside the
> 'struct packet_reader' is still unclear; could the packet_reader create
> its own (strbuf) buffers so that the contract around them (who is allowed
> to write to them; who is responsible for freeing them) is more obvious?

Just to be clear: I sent that review after you sent this patch, so
there should not have been any reason for me to expect the q's and
nits to magically not apply. ;-)

Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Jonathan Nieder]

Hi,

Brandon Williams 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 
> ---
>  pkt-line.c | 59 +++
>  pkt-line.h | 58 ++
>  2 files changed, 117 insertions(+)

I like it!

The questions and nits from
https://public-inbox.org/git/20180213004937.gb42...@aiede.svl.corp.google.com/
still apply.  In particular, the ownership of the buffers inside the
'struct packet_reader' is still unclear; could the packet_reader create
its own (strbuf) buffers so that the contract around them (who is allowed
to write to them; who is responsible for freeing them) is more obvious?

Thanks,
Jonathan

Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Subject: pkt-line: introduce struct packet_reader

nit: this subject line doesn't describe what the purpose/intent behind
the patch is.  Maybe something like

pkt-line: allow peeking at a packet line without consuming it

would make it clearer.

> 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()'.

Makes sense.  The packet_reader owns a buffer to support the peek
operation and make buffer reuse a little easier.

[...]
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t 
> *src_len, int *size);
>   */
>  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>  
> +struct packet_reader {
> + /* source file descriptor */
> + int fd;
> +
> + /* source buffer and its size */
> + char *src_buffer;
> + size_t src_len;

Can or should this be a strbuf?

> +
> + /* buffer that pkt-lines are read into and its size */
> + char *buffer;
> + unsigned buffer_size;

Likewise.

> +
> + /* options to be used during reads */
> + int options;

What option values are possible?

> +
> + /* status of the last read */
> + enum packet_read_status status;

This reminds me of FILE*'s status value, ferror, etc.  I'm mildly
nervous about it --- it encourages a style of error handling where you
ignore errors from an individual operation and hope the recorded
status later has the most relevant error.

I think it is being used to support peek --- you need to record the
error to reply it.  Is that right?  I wonder if it would make sense to
structure as

struct packet_read_result last_line_read;
};

struct packet_read_result {
enum packet_read_status status;

const char *line;
int len;
};

What you have here also seems fine.  I think what would help most
for readability is if the comment mentioned the purpose --- e.g.

/* status of the last read, to support peeking */

Or if the contract were tied to the purpose:

/* status of the last read, only valid if line_peeked is true */

[...]
> +/*
> + * Initialize a 'struct packet_reader' object which is an
> + * abstraction around the 'packet_read_with_status()' function.
> + */
> +extern void packet_reader_init(struct packet_reader *reader, int fd,
> +char *src_buffer, size_t src_len,
> +int options);

This comment doesn't describe how I should use the function.  Is the
intent to point the reader to packet_read_with_status for more details
about the arguments?

Can src_buffer be a const char *?

[...]
> +/*
> + * Perform a packet read and return the status of the read.

nit: s/Perform a packet read/Read one pkt-line/

> + * The values of 'pktlen' and 'line' are updated based on the status of the
> + * read as follows:
> + *
> + * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
> + * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
> + *  'line' is set to point at the read line
> + * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
> + */
> +extern enum packet_read_status packet_reader_read(struct packet_reader 
> *reader);

This is reasonable.  As described above an alternative would be
possible to have a separate packet_read_result output parameter but
the interface described here looks pretty easy/pleasant to use.

> +
> +/*
> + * Peek the next packet line without consuming it and return the status.

nit: s/Peek/Peek at/, or s/Peek/Read/

> + * The next call to 'packet_reader_read()' will perform a read of the same 
> line
> + * that was peeked, consuming the line.

nit: s/peeked/peeked at/

> + *
> + * Peeking multiple times without calling 'packet_reader_read()' will return
> + * the same result.
> + */
> +extern enum packet_read_status packet_reader_peek(struct packet_reader 
> *reader);

Nice.

[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -406,3 +406,62 @@ 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,
> + int options)

This comment looks like it's attached to packet_reader_init, but it's
meant to be attached to the whole collection.  It's possible that this
title-above-multiple-functions won't be maintained, but that's okay.

> +{
> + 

[PATCH v3 02/35] pkt-line: introduce struct packet_reader

[Brandon Williams]

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 
---
 pkt-line.c | 59 +++
 pkt-line.h | 58 ++
 2 files changed, 117 insertions(+)

diff --git a/pkt-line.c b/pkt-line.c
index af0d2430f..4fc9ad4b0 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -406,3 +406,62 @@ 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,
+   int options)
+{
+   memset(reader, 0, sizeof(*reader));
+
+   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 = options;
+}
+
+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,
+>src_buffer,
+>src_len,
+reader->buffer,
+reader->buffer_size,
+>pktlen,
+reader->options);
+
+   switch (reader->status) {
+   case PACKET_READ_EOF:
+   reader->pktlen = -1;
+   reader->line = NULL;
+   break;
+   case PACKET_READ_NORMAL:
+   reader->line = reader->buffer;
+   break;
+   case PACKET_READ_FLUSH:
+   reader->pktlen = 0;
+   reader->line = NULL;
+   break;
+   }
+
+   return reader->status;
+}
+
+enum packet_read_status packet_reader_peek(struct packet_reader *reader)
+{
+   /* Only allow peeking a single line */
+   if (reader->line_peeked)
+   return reader->status;
+
+   /* Peek a line by reading it and setting peeked flag */
+   packet_reader_read(reader);
+   reader->line_peeked = 1;
+   return reader->status;
+}
diff --git a/pkt-line.h b/pkt-line.h
index 06c468927..7d9f0e537 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t 
*src_len, int *size);
  */
 ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
 
+struct packet_reader {
+   /* source file descriptor */
+   int fd;
+
+   /* source buffer and its size */
+   char *src_buffer;
+   size_t src_len;
+
+   /* buffer that pkt-lines are read into and its size */
+   char *buffer;
+   unsigned buffer_size;
+
+   /* options to be used during reads */
+   int options;
+
+   /* status of the last read */
+   enum packet_read_status status;
+
+   /* length of data read during the last read */
+   int pktlen;
+
+   /* the last line read */
+   const char *line;
+
+   /* indicates if a line has been peeked */
+   int line_peeked;
+};
+
+/*
+ * Initialize a 'struct packet_reader' object which is an
+ * abstraction around the 'packet_read_with_status()' function.
+ */
+extern void packet_reader_init(struct packet_reader *reader, int fd,
+  char *src_buffer, size_t src_len,
+  int options);
+
+/*
+ * Perform a packet read and return the status of the read.
+ * The values of 'pktlen' and 'line' are updated based on the status of the
+ * read as follows:
+ *
+ * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
+ * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
+ *'line' is set to point at the read line
+ * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
+ */
+extern enum packet_read_status packet_reader_read(struct packet_reader 
*reader);
+
+/*
+ * Peek the next packet line without consuming it and return the status.
+ * The next call to 'packet_reader_read()' will perform a read of the same line
+ * that was peeked, consuming the line.
+ *
+ * Peeking multiple times without calling 'packet_reader_read()' will return
+ * the same result.
+ */
+extern enum packet_read_status