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 [off-list ref] wrote:

W dniu 30.07.2016 o 01:37, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>

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?

quoted

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)

quoted

Signed-off-by: Lars Schneider <redacted>
---
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.

quoted

+	return ret;
+}

Nice clean function, thanks to extracting set_packet_header().

quoted

+
+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!

quoted

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

quoted

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

quoted

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

quoted

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

quoted

extern char packet_buffer[LARGE_PACKET_MAX];

#endif