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

[Lars Schneider <larsxschneider@gmail.com>]

> 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?

How about this:

[...]

If the caller creates the buffer then a proper pkt-line buffer with header
and data section can be created. The PKTLINE_DATA_START macro can be used 
to skip the header section and write directly to the data section (PKTLINE_DATA_LEN 
bytes would be the maximum). direct_packet_write() would take this buffer, 
fill the pkt-line header section with the appropriate data length value and 
write the entire buffer.

If the caller does not create the buffer, and consequently cannot leave room
for the pkt-line header, then direct_packet_write_data() can be used. This 
function creates an extra buffer for the pkt-line header and afterwards writes
the header buffer and the data buffer with two consecutive write calls.

---
Is that more clear?

> 
>> 
>> 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?

Yes, git grep "gentle" reveals:

wrapper.c:static int memory_limit_check(size_t size, int gentle)
object.c:int type_from_string_gently(const char *str, ssize_t len, int gentle)


>> 
>> 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.

Agreed!

> 
>> +{
>> +	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.

Right, it would be optimized. However, I don't like the 4 there either. OK to use a macro
instead? PKTLINE_HEADER_LEN ?


>> +			!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")
>  +		);


Original:
		ret = (
			!write_or_whine_pipe(fd, hdr, sizeof(hdr), "pkt-line header") ||
			!write_or_whine_pipe(fd, data, size, "pkt-line data")
		);

Well, if the first write call fails (return == 0), then it is negated and evaluates to true.
I would think the second call is not evaluated, then?!

CPP reference:
"For the built-in logical OR operator, the result is true if either the first or the second 
operand (or both) is true. If the firstoperand is true, the second operand is not evaluated."
http://en.cppreference.com/w/cpp/language/operator_logical

Should I make this more explicit with a if clause?
 

>> +	} 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...

I think so, too.


> 
>> +	}
>> +	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.

Good point, I should use them to check for the maximal packet length!

Thanks,
Lars

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