Re: [PATCH] pkt-line: extract out PACKET_HEADER_SIZE

[Denton Liu <liu.denton@gmail.com>]

On Sat, Jun 13, 2020 at 09:23:06PM +0700, Đoàn Trần Công Danh wrote:
> > diff --git a/pkt-line.c b/pkt-line.c
> > index 8f9bc68ee2..245a56712f 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -87,43 +87,43 @@ static void packet_trace(const char *buf, unsigned int len, int write)
> >   */
> >  void packet_flush(int fd)
> >  {
> > -	packet_trace("0000", 4, 1);
> > -	if (write_in_full(fd, "0000", 4) < 0)
> > +	packet_trace("0000", PACKET_HEADER_SIZE, 1);
> > +	if (write_in_full(fd, "0000", PACKET_HEADER_SIZE) < 0)
> 
> I think the magic number 4 is easier to follow than some macro
> PACKET_HEADER_SIZE defined elsewhere. Because reader may judge that is
> the size of "0000"
> 
> How about (this ugly code):
> 
> 	packet_trace("0000", sizeof "0000" - 1, 1);
> 	if (write_in_full(fd, "0000", sizeof "0000" - 1) < 0)

Chris Torek mentioned something similar off-list:
> This seems sensible, but at the same time, the string literals must
> have the right size.  For instance:
> 
[...the diff above...]
> 
> So if you're going to do this, it might be a good idea to replace
> the string literals "0000", "0001", etc, with defined values as well.

Between the two suggested approaches, with your approach, it's
immediately obvious what the values of the packets are but, on the other
hand, with Chris's approach, we can consolidate the "magic" string
literals together into a definition in a single-place.

In this case, the strings don't seem too magical and I think that the
people on this list are generally against over-abstracting and would
consider it more readable to leave the literals in place. (If this were
my own project, I'd take Chris's approach, though, since I try to avoid
bare literals like the plague.)