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

["Torsten Bögershausen" <tboegi@web.de>]

[snip]
>> 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, ...);"
>
>>> +{
>>> +	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");


Does this work ?
I would have expected
packet_writel(fd, "line one", "line two", "line n"), NULL;

>
> 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.
>
What is wrong with

int packet_write_fmt_gently(int fd, const char *fmt, ...)
and we use it like this:
if packet_write_fmt_gently(fd, "%s%s%s", "line one", "line two", "line n")



(Or do we need another one like)
int packet_write_fmt_gently_flush(int fd, const char *fmt, ...)

Sorry if I am lost here