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

[This response might have been invalidated by v4]

W dniu 01.08.2016 o 14:00, Lars Schneider pisze:

quoted

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

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?

Yes, I think it is more clear.  

The only thing that could be improved is to perhaps instead of using

  "then a proper pkt-line buffer with header and data section can be created"

it might be more clear to write

  "then a proper pkt-line buffer with data section and a place for pkt-line header"
 

quoted

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 ?

Did you mean 

    +	char hdr[PKTLINE_HEADER_LEN];
    +	set_packet_header(hdr, sizeof(hdr) + size);

 

quoted

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

This is true both for || and for &&, as in C logical boolean operators
short-circuit.

Should I make this more explicit with a if clause?

No need.

-- 
Jakub Narębski