Re: [PATCH v3 01/35] pkt-line: introduce packet_read_with_status
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Feb 13, 2018, 00:25 UTC
- Message-ID
- <20180213002554.GA42272@aiede.svl.corp.google.com>
- In-Reply-To
- <20180207011312.189834-2-bmwill@google.com>
Hi,
Brandon Williams wrote:
Show 12 quoted lines
> 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.
[...]
Show 11 quoted lines
> --- 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?
Show 11 quoted lines
> {
> - 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.
Show 10 quoted lines
> 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("0000", 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?
[...]
Show 30 quoted lines
> @@ -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, &pktlen,
> + 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()'?)
Show 17 quoted lines
> --- 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 numbering were left to the compiler (0, 1, 2).
Thanks, Jonathan