Re: [PATCH v4 01/35] pkt-line: introduce packet_read_with_status
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Mar 13, 2018, 19:35 UTC
- Message-ID
- <20180313123540.d6e416220329bdc172472dc6@google.com>
- In-Reply-To
- <20180228232252.102167-2-bmwill@google.com>
On Wed, 28 Feb 2018 15:22:18 -0800 Brandon Williams <bmwill@google.com> wrote:
Show 15 quoted lines
> + 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;
> + } 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);The cast to unsigned is safe, since len was at least 4 before "len -= 4". I can't think of a better way to write this to make that more obvious, though.
Show 15 quoted lines
> +/*
> + * 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);jrnieder said in [1], referring to the definition of enum packet_read_status:
> 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).
I checked the result of the entire patch set and the only callers seem to be packet_read() (modified in this patch) and the soon-to-be-introduced packet_reader_read(). So not only can the numbering be left to the compiler, this function can (and should) be marked static as well (and the enum definition moved to .c), since I think that future development should be encouraged to use packet_reader.
The commit message would also thus need to be rewritten, since this becomes more of a refactoring into a function with a more precisely specified return type, to be used both by the existing packet_read() and a soon-to-be-introduced packet_reader_read().
[1] https://public-inbox.org/git/20180213002554.GA42272@aiede.svl.corp.google.com/