From: Lars Schneider <larsxschneider@gmail.com>
To: "Jakub Narębski" <jnareb@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
tboegi@web.de, mlbright@gmail.com, Eric Wong <e@80x24.org>,
Jeff King <peff@peff.net>
Subject: Re: [PATCH v3 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data()
Date: Fri, 5 Aug 2016 14:02:03 +0200 [thread overview]
Message-ID: <B113DD3B-A8AD-452A-B1E6-A92C84665D65@gmail.com> (raw)
In-Reply-To: <e8b550ed-1765-764f-49e5-72e5a609d936@gmail.com>
> 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"
OK. I changed it to
"If the caller has control over the buffer creation then a proper pkt-line
buffer with header and data section can be allocated. 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)..."
However, I am not yet sure if I can/will keep this patch:
http://public-inbox.org/git/xmqqeg645x6b.fsf%40gitster.mtv.corp.google.com/
>
>>>> +{
>>>> + 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);
yes!
>>>> + !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.
True. That's why I did not get your "de morganize" it comment... what would de morgan change?
>
>> Should I make this more explicit with a if clause?
>
> No need.
OK
Thanks,
Lars
next prev parent reply other threads:[~2016-08-05 12:02 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20160727000605.49982-1-larsxschneider%40gmail.com/>
2016-07-29 23:37 ` [PATCH v3 00/10] Git filter protocol larsxschneider
2016-07-29 23:37 ` [PATCH v3 01/10] pkt-line: extract set_packet_header() larsxschneider
2016-07-30 10:30 ` Jakub Narębski
2016-08-01 11:33 ` Lars Schneider
2016-08-03 20:05 ` Jakub Narębski
2016-08-05 11:52 ` Lars Schneider
2016-07-29 23:37 ` [PATCH v3 02/10] pkt-line: add direct_packet_write() and direct_packet_write_data() larsxschneider
2016-07-30 10:49 ` Jakub Narębski
2016-08-01 12:00 ` Lars Schneider
2016-08-03 20:12 ` Jakub Narębski
2016-08-05 12:02 ` Lars Schneider [this message]
2016-07-29 23:37 ` [PATCH v3 03/10] pkt-line: add packet_flush_gentle() larsxschneider
2016-07-30 12:04 ` Jakub Narębski
2016-08-01 12:28 ` Lars Schneider
2016-07-31 20:36 ` Torstem Bögershausen
2016-07-31 21:45 ` Lars Schneider
2016-08-02 19:56 ` Torsten Bögershausen
2016-08-05 9:59 ` Lars Schneider
2016-07-29 23:37 ` [PATCH v3 04/10] pkt-line: call packet_trace() only if a packet is actually send larsxschneider
2016-07-30 12:29 ` Jakub Narębski
2016-08-01 12:18 ` Lars Schneider
2016-08-03 20:15 ` Jakub Narębski
2016-07-29 23:37 ` [PATCH v3 05/10] pack-protocol: fix maximum pkt-line size larsxschneider
2016-07-30 13:58 ` Jakub Narębski
2016-08-01 12:23 ` Lars Schneider
2016-07-29 23:37 ` [PATCH v3 06/10] run-command: add clean_on_exit_handler larsxschneider
2016-07-30 9:50 ` Johannes Sixt
2016-08-01 11:14 ` Lars Schneider
2016-08-02 5:53 ` Johannes Sixt
2016-08-02 7:41 ` Lars Schneider
2016-07-29 23:37 ` [PATCH v3 07/10] convert: quote filter names in error messages larsxschneider
2016-07-29 23:37 ` [PATCH v3 08/10] convert: modernize tests larsxschneider
2016-07-29 23:38 ` [PATCH v3 09/10] convert: generate large test files only once larsxschneider
2016-07-29 23:38 ` [PATCH v3 10/10] convert: add filter.<driver>.process option larsxschneider
2016-07-30 22:05 ` Jakub Narębski
2016-07-31 9:42 ` Jakub Narębski
2016-07-31 19:49 ` Lars Schneider
2016-07-31 22:59 ` Jakub Narębski
2016-08-01 13:32 ` Lars Schneider
2016-08-03 18:30 ` Designing the filter process protocol (was: Re: [PATCH v3 10/10] convert: add filter.<driver>.process option) Jakub Narębski
2016-08-05 10:32 ` Lars Schneider
2016-08-06 18:24 ` Lars Schneider
2016-08-03 22:47 ` [PATCH v3 10/10] convert: add filter.<driver>.process option Jakub Narębski
2016-07-31 22:19 ` Jakub Narębski
2016-08-01 17:55 ` Lars Schneider
2016-08-04 0:42 ` Jakub Narębski
2016-08-03 13:10 ` Lars Schneider
2016-08-04 10:18 ` Jakub Narębski
2016-08-05 13:20 ` Lars Schneider
2016-08-03 16:42 ` [PATCH v4 00/12] Git filter protocol larsxschneider
2016-08-03 16:42 ` [PATCH v4 01/12] pkt-line: extract set_packet_header() larsxschneider
2016-08-03 20:18 ` Junio C Hamano
2016-08-03 21:12 ` Jeff King
2016-08-03 21:27 ` Jeff King
2016-08-04 16:14 ` Junio C Hamano
2016-08-05 14:55 ` Lars Schneider
2016-08-05 16:31 ` Junio C Hamano
2016-08-05 17:31 ` Lars Schneider
2016-08-05 17:41 ` Junio C Hamano
2016-08-03 21:56 ` Lars Schneider
2016-08-03 16:42 ` [PATCH v4 02/12] pkt-line: add direct_packet_write() and direct_packet_write_data() larsxschneider
2016-08-03 16:42 ` [PATCH v4 03/12] pkt-line: add packet_flush_gentle() larsxschneider
2016-08-03 21:39 ` Jeff King
2016-08-03 22:56 ` [PATCH 0/7] minor trace fixes and cosmetic improvements Jeff King
2016-08-03 22:56 ` [PATCH 1/7] trace: handle NULL argument in trace_disable() Jeff King
2016-08-03 22:58 ` [PATCH 2/7] trace: stop using write_or_whine_pipe() Jeff King
2016-08-03 22:58 ` [PATCH 3/7] trace: use warning() for printing trace errors Jeff King
2016-08-04 20:41 ` Junio C Hamano
2016-08-04 21:21 ` Jeff King
2016-08-04 21:28 ` Junio C Hamano
2016-08-05 7:56 ` Jeff King
2016-08-05 7:59 ` Christian Couder
2016-08-05 18:41 ` Junio C Hamano
2016-08-03 23:00 ` [PATCH 4/7] trace: cosmetic fixes for error messages Jeff King
2016-08-04 20:42 ` Junio C Hamano
2016-08-05 8:00 ` Jeff King
2016-08-03 23:00 ` [PATCH 5/7] trace: correct variable name in write() error message Jeff King
2016-08-03 23:01 ` [PATCH 6/7] trace: disable key after write error Jeff King
2016-08-04 20:45 ` Junio C Hamano
2016-08-04 21:22 ` Jeff King
2016-08-05 7:58 ` Jeff King
2016-08-03 23:01 ` [PATCH 7/7] write_or_die: drop write_or_whine_pipe() Jeff King
2016-08-03 23:04 ` [PATCH 0/7] minor trace fixes and cosmetic improvements Jeff King
2016-08-04 16:16 ` [PATCH v4 03/12] pkt-line: add packet_flush_gentle() Junio C Hamano
2016-08-03 16:42 ` [PATCH v4 04/12] pkt-line: call packet_trace() only if a packet is actually send larsxschneider
2016-08-03 16:42 ` [PATCH v4 05/12] pkt-line: add functions to read/write flush terminated packet streams larsxschneider
2016-08-03 16:42 ` [PATCH v4 06/12] pack-protocol: fix maximum pkt-line size larsxschneider
2016-08-03 16:42 ` [PATCH v4 07/12] run-command: add clean_on_exit_handler larsxschneider
2016-08-03 21:24 ` Jeff King
2016-08-03 22:15 ` Lars Schneider
2016-08-03 22:53 ` Jeff King
2016-08-03 23:09 ` Lars Schneider
2016-08-03 23:15 ` Jeff King
2016-08-05 13:08 ` Lars Schneider
2016-08-05 21:19 ` Torsten Bögershausen
2016-08-05 21:50 ` Lars Schneider
2016-08-03 16:42 ` [PATCH v4 08/12] convert: quote filter names in error messages larsxschneider
2016-08-03 16:42 ` [PATCH v4 09/12] convert: modernize tests larsxschneider
2016-08-03 16:42 ` [PATCH v4 10/12] convert: generate large test files only once larsxschneider
2016-08-03 16:42 ` [PATCH v4 11/12] convert: add filter.<driver>.process option larsxschneider
2016-08-03 17:45 ` Junio C Hamano
2016-08-03 21:48 ` Lars Schneider
2016-08-03 22:46 ` Jeff King
2016-08-05 12:53 ` Lars Schneider
2016-08-03 20:29 ` Junio C Hamano
2016-08-03 21:37 ` Lars Schneider
2016-08-03 21:43 ` Junio C Hamano
2016-08-03 22:01 ` Lars Schneider
2016-08-05 21:34 ` Torsten Bögershausen
2016-08-05 21:49 ` Lars Schneider
2016-08-05 22:06 ` Junio C Hamano
2016-08-05 22:27 ` Jeff King
2016-08-06 11:55 ` Lars Schneider
2016-08-06 12:14 ` Jeff King
2016-08-06 18:19 ` Lars Schneider
2016-08-08 15:02 ` Jeff King
2016-08-08 16:21 ` Lars Schneider
2016-08-08 16:26 ` Jeff King
2016-08-06 20:40 ` Torsten Bögershausen
2016-08-03 16:42 ` [PATCH v4 12/12] convert: add filter.<driver>.process shutdown command option larsxschneider
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B113DD3B-A8AD-452A-B1E6-A92C84665D65@gmail.com \
--to=larsxschneider@gmail.com \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jnareb@gmail.com \
--cc=mlbright@gmail.com \
--cc=peff@peff.net \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).