Re: [PATCH v8 06/11] pkt-line: add packet_write_gently()

From: Jakub Narębski <jnareb@gmail.com>
Date: Sep 25, 2016, 11:26 UTC
Message-ID: <55c5a86e-4055-f36d-bbf1-e8bae919088a@gmail.com>
In-Reply-To: <20160920190247.82189-7-larsxschneider@gmail.com>

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

Show 9 quoted lines

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> packet_write_fmt_gently() uses format_packet() which lets the caller
> only send string data via "%s". That means it cannot be used for
> arbitrary data that may contain NULs.
> 
> Add packet_write_gently() which writes arbitrary data and does not die
> in case of an error. The function is used by other pkt-line functions in
> a subsequent patch.

Nice; obviously needed for sending binary data.

I wonder how send-pack / receive-pack handles sending binary files. Though this is outside of scope of this patch series, it is something to think about for later.

Show 15 quoted lines

> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  pkt-line.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 19f0271..fc0ac12 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,22 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>  	return status;
>  }
>  
> +static int packet_write_gently(const int fd_out, const char *buf, size_t size)

I'm not sure what naming convention the rest of Git uses, but isn't it more like '*data' rather than '*buf' here?

> +{
> +	static char packet_write_buffer[LARGE_PACKET_MAX];

I think there should be warning (as a comment before function declaration, or before function definition), that packet_write_gently() is not thread-safe (nor reentrant, but the latter does not matter here, I think).

Thread-safe vs reentrant: http://stackoverflow.com/a/33445858/46058

This is not something terribly important; I guess git code has tons of functions not marked as thread-unsafe...

> +
> +	if (size > sizeof(packet_write_buffer) - 4) {

First, wouldn't the following be more readable:

  +	if (size + 4 > LARGE_PACKET_MAX) {

> +		return error("packet write failed - data exceeds max packet size");
> +	}

Second, CodingGuidelines is against using braces (blocks) for one line conditionals: "We avoid using braces unnecessarily."

But this is just me nitpicking.

Show 5 quoted lines

> +	packet_trace(buf, size, 1);
> +	size += 4;
> +	set_packet_header(packet_write_buffer, size);
> +	memcpy(packet_write_buffer + 4, buf, size - 4);
> +	if (write_in_full(fd_out, packet_write_buffer, size) == size)

Hmmm... in some places we use original size, in others (original) size + 4; perhaps it would be more readable to add a new local temporary variable

	size_t full_size = size + 4;

Or perhaps use 'data_size' and 'packet_size', where 'packet_size = data_size + 4'. But that might be too chatty for variable names ;-)

> +		return 0;
> +	return error("packet write failed");
> +}

Compared to previous iterations, where there were two versions of this function, IIRC sharing no common code: one taking buffer which had to be with place for packet size info, one with a separate local buffer for packet size only and using two writes. This version uses static buffer (thus not thread-safe, I think; and not reentrant), and memcpy.

Anyway, if reentrant / thread-safe version would be required, or not doing memcpy turns out to be important with respect to performance, we can provide with the *_r version:

  static int packet_write_gently_r(const int fd_out, const char *data, size_t size,
                                   char *restrict buf)

We can check if 'buf + 4 == data', and if it is, we can skip memcpy() as an optimization.

This is something for the future, but not very important for having this patch series accepted.

Show 5 quoted lines

> +
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  {
>  	va_list args;
>

Best,

-- 
Jakub Narębski