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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux