Re: [PATCH v3 01/35] pkt-line: introduce packet_read_with_status

[Jonathan Nieder]

Hi,

Brandon Williams 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.

Makes sense, thanks.  Using an enum return value is less opaque, too.

[...]
> --- 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)

This function definition straddles two worlds: it is line-wrapped as
though there are a limited number of columns, but it goes far past 80
columns.

Can "make style" or a similar tool take care of rewrapping it?


>  {
> - 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;
> +

EOF is indeed the only error that get_packet_data can return.

Could be worth a doc comment on get_packet_data to make that clearer.
It's not too important since it's static, though.

>   len = packet_length(linelen);
> - if (len < 0)
> +
> + if (len < 0) {
>   die("protocol error: bad line length character: %.4s", linelen);
> - if (!len) {
> + } else if (!len) {
>   packet_trace("", 4, 0);
> - return 0;
> + return PACKET_READ_FLUSH;

The advertised change. Makes sense.

[...]
> - if (len >= size)
> + if ((unsigned)len >= size)
>   die("protocol error: bad line length %d", len);

The comparison is safe since we just checked that len >= 0.

Is there some static analysis that can make this kind of operation
easier?

[...]
> @@ -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, ,
> +  options);
> + switch (status) {
> + case PACKET_READ_EOF:
> + pktlen = -1;
> + break;
> + case PACKET_READ_NORMAL:
> + break;
> + case PACKET_READ_FLUSH:
> + pktlen = 0;
> + break;
> + }
> +
> + return pktlen;

nit: can simplify by avoiding the status temporary:

int pktlen;

switch (packet_read_with_status(...)) {
case PACKET_READ_EOF:
return -1;
case PACKET_READ_FLUSH:
return 0;
case PACKET_READ_NORMAL:
return pktlen;
}

As a bonus, that lets static analyzers check that the cases are
exhaustive.  (On the other hand, C doesn't guarantee that an enum can
only have the values listed as enumerators.  Did we end up figuring
out a way to handle that, beyond always including a 'default: BUG()'?)

> --- 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,
> +};

nit: do any callers treat the return value as a number?  It would be
less magical if the 

[PATCH v3 01/35] pkt-line: introduce packet_read_with_status

[Brandon Williams]

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 
---
 pkt-line.c | 57 +++--
 pkt-line.h | 15 +++
 2 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 2827ca772..af0d2430f 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)
+
+   if (len < 0) {
die("protocol error: bad line length character: %.4s", linelen);
-   if (!len) {
+   } else if (!len) {
packet_trace("", 4, 0);
-   return 0;
+   return PACKET_READ_FLUSH;
+   } else if (len < 4) {
+   die("protocol error: bad line length %d", len);
}
+
len -= 4;
-   if (len >= size)
+   if ((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, ,
+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.16.0.rc1.238.g530d649a79-goog