Re: [PATCH 3/7] avoid "write_in_full(fd, buf, len) != len" pattern
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Sep 13, 2017, 21:14 UTC
- Message-ID
- <20170913211430.GK27425@aiede.mtv.corp.google.com>
- In-Reply-To
- <20170913171603.3whuql7l5da3ynrn@sigill.intra.peff.net>
Jeff King wrote:
Show 11 quoted lines
> The return value of write_in_full() is either "-1", or the > requested number of bytes[1]. If we make a partial write > before seeing an error, we still return -1, not a partial > value. This goes back to f6aa66cb95 (write_in_full: really > write in full or return error on disk full., 2007-01-11). > > So checking anything except "was the return value negative" > is pointless. And there are a couple of reasons not to do > so: > > 1. It can do a funny signed/unsigned comparison. If your
[...]
Show 10 quoted lines
> 2. Checking for a negative value is shorter to type, > especially when the length is an expression. > > 3. Linus says so. In d34cf19b89 (Clean up write_in_full() > users, 2007-01-11), right after the write_in_full() > semantics were changed, he wrote: > > I really wish every "write_in_full()" user would just > check against "<0" now, but this fixes the nasty and > stupid ones.
Ok, you convinced me.
Should we add a comment to cache.h as well encouraging this?
[...]
Show 11 quoted lines
> [1] A careful reader may notice there is one way that > write_in_full() can return a different value. If we ask > write() to write N bytes and get a return value that is > _larger_ than N, we could return a larger total. But > besides the fact that this would imply a totally broken > version of write(), it would already invoke undefined > behavior. Our internal remaining counter is an unsigned > size_t, which means that subtracting too many byte will > wrap it around to a very large number. So we'll instantly > begin reading off the end of the buffer, trying to write > gigabytes (or petabytes) of data.
This footnote just leaves me more confused, since as you mention, write() never would return a value greater than N. Are you saying we need to defend against a broken platform where that isn't true?
Show 19 quoted lines
> Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/receive-pack.c | 2 +- > builtin/rerere.c | 2 +- > builtin/unpack-file.c | 2 +- > config.c | 4 ++-- > diff.c | 2 +- > fast-import.c | 2 +- > http-backend.c | 4 ++-- > ll-merge.c | 2 +- > read-cache.c | 6 +++--- > refs.c | 2 +- > refs/files-backend.c | 8 ++++---- > rerere.c | 2 +- > shallow.c | 6 +++--- > t/helper/test-delta.c | 2 +- > transport-helper.c | 5 ++--- > wrapper.c | 2 +- > 16 files changed, 26 insertions(+), 27 deletions(-)
All of these look correctly done.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>