Re: [PATCH v3 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data()

["Jakub Narębski" <jnareb@gmail.com>]

W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:
> From: Lars Schneider <larsxschneider@gmail.com>
> 
> Sometimes pkt-line data is already available in a buffer and it would
> be a waste of resources to write the packet using packet_write() which
> would copy the existing buffer into a strbuf before writing it.
> 
> If the caller has control over the buffer creation then the
> PKTLINE_DATA_START macro can be used to skip the header and write
> directly into the data section of a pkt-line (PKTLINE_DATA_LEN bytes
> would be the maximum). direct_packet_write() would take this buffer,
> adjust the pkt-line header and write it.
> 
> If the caller has no control over the buffer creation then
> direct_packet_write_data() can be used. This function creates a pkt-line
> header. Afterwards the header and the data buffer are written using two
> consecutive write calls.

I don't quite understand what do you mean by "caller has control
over the buffer creation".  Do you mean that caller either can write
over the buffer, or cannot overwrite the buffer?  Or do you mean that
caller either can allocate buffer to hold header, or is getting
only the data?

> 
> Both functions have a gentle parameter that indicates if Git should die
> in case of a write error (gentle set to 0) or return with a error (gentle
> set to 1).

So they are *_maybe_gently(), isn't it ;-)?  Are there any existing
functions in Git codebase that take 'gently' / 'strict' / 'die_on_error'
parameter?

> 
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>  pkt-line.c | 30 ++++++++++++++++++++++++++++++
>  pkt-line.h |  5 +++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index 445b8e1..6fae508 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -135,6 +135,36 @@ void packet_write(int fd, const char *fmt, ...)
>  	write_or_die(fd, buf.buf, buf.len);
>  }
>  
> +int direct_packet_write(int fd, char *buf, size_t size, int gentle)
> +{
> +	int ret = 0;
> +	packet_trace(buf + 4, size - 4, 1);
> +	set_packet_header(buf, size);
> +	if (gentle)
> +		ret = !write_or_whine_pipe(fd, buf, size, "pkt-line");
> +	else
> +		write_or_die(fd, buf, size);

Hmmm... in gently case we get the information in the warning that
it is about "pkt-line", which is missing from !gently case.  But
it is probably not important.

> +	return ret;
> +}

Nice clean function, thanks to extracting set_packet_header().

> +
> +int direct_packet_write_data(int fd, const char *buf, size_t size, int gentle)

I would name the parameter 'data', rather than 'buf'; IMVHO it
better describes it.

> +{
> +	int ret = 0;
> +	char hdr[4];
> +	set_packet_header(hdr, sizeof(hdr) + size);
> +	packet_trace(buf, size, 1);
> +	if (gentle) {
> +		ret = (
> +			!write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line header") ||

You can write '4' here, no need for sizeof(hdr)... though compiler would
optimize it away.

> +			!write_or_whine_pipe(fd, buf, size, "pkt-line data")
> +		);

Do we want to try to write "pkt-line data" if "pkt-line header" failed?
If not, perhaps De Morgan-ize it

  +		ret = !(
  +			write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line header") &&
  +			write_or_whine_pipe(fd, buf, size, "pkt-line data")
  +		);


> +	} else {
> +		write_or_die(fd, hdr, sizeof(hdr));
> +		write_or_die(fd, buf, size);

I guess these two writes (here and in 'gently' case) are unavoidable...

> +	}
> +	return ret;
> +}
> +
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...)
>  {
>  	va_list args;
> diff --git a/pkt-line.h b/pkt-line.h
> index 3cb9d91..02dcced 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -23,6 +23,8 @@ void packet_flush(int fd);
>  void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>  void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> +int direct_packet_write(int fd, char *buf, size_t size, int gentle);
> +int direct_packet_write_data(int fd, const char *buf, size_t size, int gentle);
>  
>  /*
>   * Read a packetized line into the buffer, which must be at least size bytes
> @@ -77,6 +79,9 @@ char *packet_read_line_buf(char **src_buf, size_t *src_len, int *size);
>  
>  #define DEFAULT_PACKET_MAX 1000
>  #define LARGE_PACKET_MAX 65520
> +#define PKTLINE_HEADER_LEN 4
> +#define PKTLINE_DATA_START(pkt) ((pkt) + PKTLINE_HEADER_LEN)
> +#define PKTLINE_DATA_LEN (LARGE_PACKET_MAX - PKTLINE_HEADER_LEN)

Those are not used in direct_packet_write() and direct_packet_write_data();
but they would make them more verbose and less readable.

>  extern char packet_buffer[LARGE_PACKET_MAX];
>  
>  #endif
>