On 2020-06-13 09:43:22-0400, Denton Liu <liu.denton@xxxxxxxxx> wrote:
> In pkt-line and remote-curl, we have many instances of magic `4`
> literals floating around which represent the number of bytes in the
> packet line length header. Instead of using these magic numbers, replace
> them with constant expressions.
>
> In most cases, replace the `4` with `PACKET_HEADER_SIZE`. However, in
> the case where there's a `char array[PACKET_HEADER_SIZE]` and we are
> reading data into it, replace the `4` with a `sizeof(array)` so that
> it's clear that the logic has something to do with that array.
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> pkt-line.c | 50 +++++++++++++++++++++++++-------------------------
> pkt-line.h | 6 ++++--
> remote-curl.c | 30 +++++++++++++++---------------
> 3 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 8f9bc68ee2..245a56712f 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -87,43 +87,43 @@ static void packet_trace(const char *buf, unsigned int len, int write)
> */
> void packet_flush(int fd)
> {
> - packet_trace("0000", 4, 1);
> - if (write_in_full(fd, "0000", 4) < 0)
> + packet_trace("0000", PACKET_HEADER_SIZE, 1);
> + if (write_in_full(fd, "0000", PACKET_HEADER_SIZE) < 0)
I think the magic number 4 is easier to follow than some macro
PACKET_HEADER_SIZE defined elsewhere. Because reader may judge that is
the size of "0000"
How about (this ugly code):
packet_trace("0000", sizeof "0000" - 1, 1);
if (write_in_full(fd, "0000", sizeof "0000" - 1) < 0)
> die_errno(_("unable to write flush packet"));
> }
>
> void packet_delim(int fd)
> {
> - packet_trace("0001", 4, 1);
> - if (write_in_full(fd, "0001", 4) < 0)
> + packet_trace("0001", PACKET_HEADER_SIZE, 1);
> + if (write_in_full(fd, "0001", PACKET_HEADER_SIZE) < 0)
> die_errno(_("unable to write delim packet"));
> }
>
> void packet_response_end(int fd)
> {
> - packet_trace("0002", 4, 1);
> - if (write_in_full(fd, "0002", 4) < 0)
> + packet_trace("0002", PACKET_HEADER_SIZE, 1);
> + if (write_in_full(fd, "0002", PACKET_HEADER_SIZE) < 0)
> die_errno(_("unable to write stateless separator packet"));
> }
>
> int packet_flush_gently(int fd)
> {
> - packet_trace("0000", 4, 1);
> - if (write_in_full(fd, "0000", 4) < 0)
> + packet_trace("0000", PACKET_HEADER_SIZE, 1);
> + if (write_in_full(fd, "0000", PACKET_HEADER_SIZE) < 0)
> return error(_("flush packet write failed"));
> return 0;
> }
>
> void packet_buf_flush(struct strbuf *buf)
> {
> - packet_trace("0000", 4, 1);
> - strbuf_add(buf, "0000", 4);
> + packet_trace("0000", PACKET_HEADER_SIZE, 1);
> + strbuf_add(buf, "0000", PACKET_HEADER_SIZE);
> }
>
> void packet_buf_delim(struct strbuf *buf)
> {
> - packet_trace("0001", 4, 1);
> - strbuf_add(buf, "0001", 4);
> + packet_trace("0001", PACKET_HEADER_SIZE, 1);
> + strbuf_add(buf, "0001", PACKET_HEADER_SIZE);
> }
>
> void set_packet_header(char *buf, int size)
> @@ -153,7 +153,7 @@ static void format_packet(struct strbuf *out, const char *prefix,
> die(_("protocol error: impossibly long line"));
>
> set_packet_header(&out->buf[orig_len], n);
> - packet_trace(out->buf + orig_len + 4, n - 4, 1);
> + packet_trace(out->buf + orig_len + PACKET_HEADER_SIZE, n - PACKET_HEADER_SIZE, 1);
> }
>
> static int packet_write_fmt_1(int fd, int gently, const char *prefix,
> @@ -199,13 +199,13 @@ static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> static char packet_write_buffer[LARGE_PACKET_MAX];
> size_t packet_size;
>
> - if (size > sizeof(packet_write_buffer) - 4)
> + if (size > sizeof(packet_write_buffer) - PACKET_HEADER_SIZE)
> return error(_("packet write failed - data exceeds max packet size"));
>
> packet_trace(buf, size, 1);
> - packet_size = size + 4;
> + packet_size = size + PACKET_HEADER_SIZE;
> set_packet_header(packet_write_buffer, packet_size);
> - memcpy(packet_write_buffer + 4, buf, size);
> + memcpy(packet_write_buffer + PACKET_HEADER_SIZE, buf, size);
> if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
> return error(_("packet write failed"));
> return 0;
> @@ -313,7 +313,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
> return ret;
> }
>
> -int packet_length(const char lenbuf_hex[4])
> +int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE])
> {
> int val = hex2chr(lenbuf_hex);
> return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
> @@ -325,9 +325,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> int options)
> {
> int len;
> - char linelen[4];
> + char linelen[PACKET_HEADER_SIZE];
>
> - if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
> + if (get_packet_data(fd, src_buffer, src_len, linelen, sizeof(linelen), options) < 0) {
> *pktlen = -1;
> return PACKET_READ_EOF;
> }
> @@ -337,22 +337,22 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> if (len < 0) {
> die(_("protocol error: bad line length character: %.4s"), linelen);
> } else if (!len) {
> - packet_trace("0000", 4, 0);
> + packet_trace("0000", PACKET_HEADER_SIZE, 0);
> *pktlen = 0;
> return PACKET_READ_FLUSH;
> } else if (len == 1) {
> - packet_trace("0001", 4, 0);
> + packet_trace("0001", PACKET_HEADER_SIZE, 0);
> *pktlen = 0;
> return PACKET_READ_DELIM;
> } else if (len == 2) {
> - packet_trace("0002", 4, 0);
> + packet_trace("0002", PACKET_HEADER_SIZE, 0);
> *pktlen = 0;
> return PACKET_READ_RESPONSE_END;
> - } else if (len < 4) {
> + } else if (len < PACKET_HEADER_SIZE) {
> die(_("protocol error: bad line length %d"), len);
> }
>
> - len -= 4;
> + len -= sizeof(linelen);
> if ((unsigned)len >= size)
> die(_("protocol error: bad line length %d"), len);
>
> @@ -370,7 +370,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
>
> if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
> starts_with(buffer, "ERR "))
> - die(_("remote error: %s"), buffer + 4);
> + die(_("remote error: %s"), buffer + PACKET_HEADER_SIZE);
>
> *pktlen = len;
> return PACKET_READ_NORMAL;
> diff --git a/pkt-line.h b/pkt-line.h
> index 5b373fe4cd..d6121b8044 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -5,6 +5,8 @@
> #include "strbuf.h"
> #include "sideband.h"
>
> +#define PACKET_HEADER_SIZE 4
> +
> /*
> * Write a packetized stream, where each line is preceded by
> * its length (including the header) as a 4-byte hex number.
> @@ -82,7 +84,7 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
> * If lenbuf_hex contains non-hex characters, return -1. Otherwise, return the
> * numeric value of the length header.
> */
> -int packet_length(const char lenbuf_hex[4]);
> +int packet_length(const char lenbuf_hex[PACKET_HEADER_SIZE]);
>
> /*
> * Read a packetized line into a buffer like the 'packet_read()' function but
> @@ -211,7 +213,7 @@ enum packet_read_status packet_reader_peek(struct packet_reader *reader);
>
> #define DEFAULT_PACKET_MAX 1000
> #define LARGE_PACKET_MAX 65520
> -#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
> +#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - PACKET_HEADER_SIZE)
> extern char packet_buffer[LARGE_PACKET_MAX];
>
> struct packet_writer {
> diff --git a/remote-curl.c b/remote-curl.c
> index 75532a8bae..bac295c5bc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -536,7 +536,7 @@ struct rpc_state {
> unsigned initial_buffer : 1;
>
> /*
> - * Whenever a pkt-line is read into buf, append the 4 characters
> + * Whenever a pkt-line is read into buf, append the PACKET_HEADER_SIZE characters
> * denoting its length before appending the payload.
> */
> unsigned write_line_lengths : 1;
> @@ -556,7 +556,7 @@ struct rpc_state {
> * rpc->buf and rpc->len if there is enough space. Returns 1 if there was
> * enough space, 0 otherwise.
> *
> - * If rpc->write_line_lengths is true, appends the line length as a 4-byte
> + * If rpc->write_line_lengths is true, appends the line length as a PACKET_HEADER_SIZE-byte
> * hexadecimal string before appending the result described above.
> *
> * Writes the total number of bytes appended into appended.
> @@ -569,8 +569,8 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,
> int pktlen_raw;
>
> if (rpc->write_line_lengths) {
> - left = rpc->alloc - rpc->len - 4;
> - buf = rpc->buf + rpc->len + 4;
> + left = rpc->alloc - rpc->len - PACKET_HEADER_SIZE;
> + buf = rpc->buf + rpc->len + PACKET_HEADER_SIZE;
> } else {
> left = rpc->alloc - rpc->len;
> buf = rpc->buf + rpc->len;
> @@ -582,7 +582,7 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,
> *status = packet_read_with_status(rpc->out, NULL, NULL, buf,
> left, &pktlen_raw, options);
> if (*status != PACKET_READ_EOF) {
> - *appended = pktlen_raw + (rpc->write_line_lengths ? 4 : 0);
> + *appended = pktlen_raw + (rpc->write_line_lengths ? PACKET_HEADER_SIZE : 0);
> rpc->len += *appended;
> }
>
> @@ -593,13 +593,13 @@ static int rpc_read_from_out(struct rpc_state *rpc, int options,
> die(_("shouldn't have EOF when not gentle on EOF"));
> break;
> case PACKET_READ_NORMAL:
> - set_packet_header(buf - 4, *appended);
> + set_packet_header(buf - PACKET_HEADER_SIZE, *appended);
> break;
> case PACKET_READ_DELIM:
> - memcpy(buf - 4, "0001", 4);
> + memcpy(buf - PACKET_HEADER_SIZE, "0001", PACKET_HEADER_SIZE);
> break;
> case PACKET_READ_FLUSH:
> - memcpy(buf - 4, "0000", 4);
> + memcpy(buf - PACKET_HEADER_SIZE, "0000", PACKET_HEADER_SIZE);
> break;
> case PACKET_READ_RESPONSE_END:
> die(_("remote server sent stateless separator"));
> @@ -682,7 +682,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> #endif
>
> struct check_pktline_state {
> - char len_buf[4];
> + char len_buf[PACKET_HEADER_SIZE];
> int len_filled;
> int remaining;
> };
> @@ -691,7 +691,7 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
> {
> while (size) {
> if (!state->remaining) {
> - int digits_remaining = 4 - state->len_filled;
> + int digits_remaining = sizeof(state->len_buf) - state->len_filled;
> if (digits_remaining > size)
> digits_remaining = size;
> memcpy(&state->len_buf[state->len_filled], ptr, digits_remaining);
> @@ -699,16 +699,16 @@ static void check_pktline(struct check_pktline_state *state, const char *ptr, si
> ptr += digits_remaining;
> size -= digits_remaining;
>
> - if (state->len_filled == 4) {
> + if (state->len_filled == sizeof(state->len_buf)) {
> state->remaining = packet_length(state->len_buf);
> if (state->remaining < 0) {
> die(_("remote-curl: bad line length character: %.4s"), state->len_buf);
> } else if (state->remaining == 2) {
> die(_("remote-curl: unexpected response end packet"));
> - } else if (state->remaining < 4) {
> + } else if (state->remaining < sizeof(state->len_buf)) {
> state->remaining = 0;
> } else {
> - state->remaining -= 4;
> + state->remaining -= sizeof(state->len_buf);
> }
> state->len_filled = 0;
> }
> @@ -804,7 +804,7 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
> curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);
> curl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);
> curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, "0000");
> - curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
> + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, PACKET_HEADER_SIZE);
> curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
> curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
> @@ -1469,7 +1469,7 @@ int cmd_main(int argc, const char **argv)
> parse_fetch(&buf);
>
> } else if (!strcmp(buf.buf, "list") || starts_with(buf.buf, "list ")) {
> - int for_push = !!strstr(buf.buf + 4, "for-push");
> + int for_push = !!strstr(buf.buf + PACKET_HEADER_SIZE, "for-push");
> output_refs(get_refs(for_push));
>
> } else if (starts_with(buf.buf, "push ")) {
> --
> 2.27.0.132.g321788e831
>
--
Danh
