RE: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()

From: Ben Peart <ben.peart@microsoft.com>
Date: Mar 27, 2017, 22:19 UTC
Message-ID: <BL2PR03MB3233616BE57BB7D911B1AAEF4330@BL2PR03MB323.namprd03.prod.outlook.com>
In-Reply-To: <7a2c73c8-bb09-12ec-dd8d-99c5363f9bb4@web.de>

Show 33 quoted lines

> -----Original Message-----
> From: Torsten Bögershausen [mailto:tboegi@web.de]
> Sent: Saturday, March 25, 2017 1:47 AM
> To: Ben Peart <peartben@gmail.com>; git@vger.kernel.org
> Cc: Ben Peart <Ben.Peart@microsoft.com>; christian.couder@gmail.com;
> larsxschneider@gmail.com
> Subject: Re: [PATCH v2 1/2] pkt-line: add packet_writel() and
> packet_read_line_gently()
> 
> On 2017-03-24 16:27, Ben Peart wrote:
> > Add packet_writel() which writes multiple lines in a single call and
> > then calls packet_flush_gently(). Add packet_read_line_gently() to
> > enable reading a line without dying on EOF.
> >
> > Signed-off-by: Ben Peart <benpeart@microsoft.com>
> > ---
> >  pkt-line.c | 31 +++++++++++++++++++++++++++++++  pkt-line.h | 11
> > +++++++++++
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/pkt-line.c b/pkt-line.c
> > index d4b6bfe076..2788aa1af6 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char
> *fmt, ...)
> >  	return status;
> >  }
> >
> > +int packet_writel(int fd, const char *line, ...)
> The name packet_writel is hard to distinguish from packet_write.
> Would packet_write_lines make more sense ?
>

Just goes to prove that there are two hard things in computer science: cache invalidation, naming things, and off-by-one errors. :)  The feedback on V1 was:

"I am hesitant to take a function that does not take any "list" type argument and yet calls itself "write_list". IOW, I'd expect something like these

	write_list(int fd, const char **lines);
	write_list(int fd, struct string_list *lines);

given that name, and not "varargs, each of which is a line".  I am tempted to suggest

	packet_writel(int fd, const char *line, ...);"

Show 36 quoted lines

> > +{
> > +	va_list args;
> > +	int err;
> > +	va_start(args, line);
> > +	for (;;) {
> > +		if (!line)
> > +			break;
> > +		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> > +			return -1;
> > +		err = packet_write_fmt_gently(fd, "%s\n", line);
> > +		if (err)
> > +			return err;
> > +		line = va_arg(args, const char*);
> > +	}
> > +	va_end(args);
> > +	return packet_flush_gently(fd);
> > +}
> > +
> I don't think that this va_start() is needed, even if it works.
> 
> int packet_write_line(int fd, const char *lines[])
> |
> 	const char *line = *lines;
> 	int err;
> 	while (line) {
> 		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> 			return -1;
> 		err = packet_write_fmt_gently(fd, "%s\n", line);
> 		if (err)
> 			return err;
> 		lines++;
> 		line = *lines;
> 	}
> 	return packet_flush_gently(fd);
> ]
>

This is a helper function to simply the common pattern of writing out a variable number of lines followed by a flush.  The usage of the function as currently implemented is:

packet_writel(fd, "line one", "line two", "line n");

which requires the use of variable number of arguments.  With your proposal that convenience is lost as you have to create an array of strings and pass that instead.  The usage just isn't as simple as the current model.