Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2020-06-13 09:43:22-0400, Denton Liu <liu.denton@gmail.com> 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@gmail.com>
> ---
>  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