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

> On 03 Aug 2016, at 22:12, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> [This response might have been invalidated by v4]
> 
> W dniu 01.08.2016 o 14:00, Lars Schneider pisze:
>>> 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:
>>>> 
>>>> 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"

> 
>>>> +{
>>>> +	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);

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