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

> On 30 Jul 2016, at 12:49, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> 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
>> 
>