Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Ben Peart <peartben@gmail.com>]

On 4/24/2017 12:21 AM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> 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 | 14 +++++++++++++-
>>   pkt-line.h | 10 ++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index d4b6bfe076..bfdb177b34 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
>>   			      PACKET_READ_CHOMP_NEWLINE);
>>   	if (dst_len)
>>   		*dst_len = len;
>> -	return len ? packet_buffer : NULL;
>> +	return (len > 0) ? packet_buffer : NULL;
>>   }
> The log does not seem to explain what this change is about.

This change was made as the result of a request in feedback from the 
previous version of the patch series which pointed out that the call to 
packet_read can return -1 in some error cases and to keep the code more 
consistent with packet_read_line_gently below.

If len < 0 then the old code would have incorrectly returned a pointer 
to a buffer with garbage while the new code correctly returns NULL 
(fixes potential bug)
if len == 0 then the code will return NULL before and after this change 
(no change in behavior)
if len > 0 then the code will return packet_buffer before and after this 
change (no change in behavior)

>
> Is this supposed to be a preliminary bugfix where this helper used
> to return a non-NULL buffer when underlying packet_read() signaled
> an error by returning a negative len?  If so, this should probably
> be a separate patch early in the series.
>
> Should len==0 be considered an error?  Especially given that
> PACKET_READ_CHOMP_NEWLINE is in use, I would expect that len==0
> should be treated similarly to positive length, i.e. the otherside
> gave us an empty line.
>
>>   char *packet_read_line(int fd, int *len_p)
>> @@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)
>>   	return packet_read_line_generic(fd, NULL, NULL, len_p);
>>   }
>>   
>> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)
> ERROR: "foo** bar" should be "foo **bar"
> #29: FILE: pkt-line.c:326:
> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)

Sorry, missed that somehow.  I'll move the space before the ** in the 
next version of the patch series.

>
>> +{
>> +	int len = packet_read(fd, NULL, NULL,
>> +			      packet_buffer, sizeof(packet_buffer),
>> +			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
>> +	if (dst_len)
>> +		*dst_len = len;
>> +	if (dst_line)
>> +		*dst_line = (len > 0) ? packet_buffer : NULL;
> I have the same doubt as above for len == 0 case.

packet_read() returns -1 when PACKET_READ_GENTLE_ON_EOF is passed and it 
hits truncated output from the remote process.  This occurs when the 
remote process exits unexpectedly which is the exact case I was fixing 
with this new function.  This requires testing len for this error 
condition so that it can correctly handle this error case, otherwise it 
would incorrectly return an invalid buffer. Since this is a new 
function, there isn't any before/after behavior comparisons but it is 
consistent with the similar packet_read_line() function.