Re: [PATCH] stream_to_pack: xread does not guarantee to read all requested bytes

[Antoine Pelisse <apelisse@gmail.com>]

On Tue, Aug 20, 2013 at 5:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> The deflate loop in bulk-checkin::stream_to_pack expects to get all bytes
>> from a file that it requests to read in a single function call. But it
>> used xread(), which does not give that guarantee. Replace it by
>> read_in_full().
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>>  The size is limited to sizeof(ibuf) == 16384 bytes, so that there
>>  should not be a problem with the unpatched code on any OS in practice.
>>  Nevertheless, this change seems reasonable from a code hygiene POV.
>>
>>  bulk-checkin.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/bulk-checkin.c b/bulk-checkin.c
>> index 6b0b6d4..118c625 100644
>> --- a/bulk-checkin.c
>> +++ b/bulk-checkin.c
>> @@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
>>
>>               if (size && !s.avail_in) {
>>                       ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
>> -                     if (xread(fd, ibuf, rsize) != rsize)
>> +                     if (read_in_full(fd, ibuf, rsize) != rsize)
>
> This is the kind of thing i was wondering and worried about with the
> other "clipped xread/xwrite" patch.  The original of this caller is
> obviously wrong.  Thanks for spotting and fixing.
>
> I wonder if there are more like this broken caller or xread and/or
> xwrite.

I was actually wondering when it's better to use xread() over
read_in_full() ? Considering that we don't know if xread() will read
the whole buffer or not, would it not be better to always use
read_in_full() ? I guess there is a drawback to this, but I'm not
exactly sure what it is.