From: Antoine Pelisse Date: Tue, 20 Aug 2013 15:16:22 GMT Subject: Re: [PATCH] stream_to_pack: xread does not guarantee to read all requested bytes Message-ID: In-Reply-To: On Tue, Aug 20, 2013 at 5:00 PM, Junio C Hamano wrote: > Johannes Sixt 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 >> --- >> 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.