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("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?
[...]
> @@ -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()'?)
> --- 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
