Re: [PATCH v3 04/10] pkt-line: call packet_trace() only if a packet is actually send
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Aug 1, 2016, 12:18 UTC
- Message-ID
- <52A5703D-AF9E-405D-A4F8-E30A7B923400@gmail.com>
- In-Reply-To
- <786f0b8e-29f0-3dd3-7bb4-5f6558f8ec84@gmail.com>
Show 10 quoted lines
> On 30 Jul 2016, at 14:29, 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> >> >> The packet_trace() call is not ideal in format_packet() as we would print > > Style; I think the following is more readable: > > The packet_trace() call in format_packet() is not ideal, as we would...
Agreed!
Show 6 quoted lines
>> a trace when a packet is formatted and (potentially) when the packet is >> actually send. This was no problem up until now because format_packet() >> was only used by one function. Fix it by moving the trace call into the >> function that actally sends the packet. > > s/actally/actually/
Thanks!
Show 11 quoted lines
> I don't buy this explanation. If you want to trace packets, you might > do it on input (when formatting packet), or on output (when writing > packet). It's when there are more than one formatting function, but > one writing function, then placing trace call in write function means > less code duplication; and of course the reverse. > > Another issue is that something may happen between formatting packet > and sending it, and we probably want to packet_trace() when packet > is actually send. > > Neither of those is visible in commit message.
The packet_trace() call in format_packet() is not ideal, as we would print a trace when a packet is formatted and (potentially) when the same packet is actually written. This was no problem up until now because packet_write(), the function that uses format_packet() and writes the formatted packet, did not trace the packet.
This developer believes that trace calls should only happen when a packet is actually written as the packet could be modified between formatting and writing. Therefore the trace call was moved from format_packet() to packet_write().
--
Better?
Show 29 quoted lines
>
>>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> pkt-line.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 1728690..32c0a34 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -126,7 +126,6 @@ static void format_packet(struct strbuf *out, const char *fmt, va_list args)
>> die("protocol error: impossibly long line");
>>
>> set_packet_header(&out->buf[orig_len], n);
>> - packet_trace(out->buf + orig_len + 4, n - 4, 1);
>> }
>>
>> void packet_write(int fd, const char *fmt, ...)
>> @@ -138,6 +137,7 @@ void packet_write(int fd, const char *fmt, ...)
>> va_start(args, fmt);
>> format_packet(&buf, fmt, args);
>> va_end(args);
>> + packet_trace(buf.buf + 4, buf.len - 4, 1);
>> write_or_die(fd, buf.buf, buf.len);
>> }
>>
>>
>