# Clean up write_in_full() users 4 messages from 2007-01-12 to 2007-01-12. Participants: Linus Torvalds, Shawn O. Pearce, Morten Welinder. Thread: https://gitlist.dev/t/6347 ## Linus Torvalds, 2007-01-12 04:23 Subject: Clean up write_in_full() users Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701112014050.3594%40woody.osdl.org ``` With the new-and-improved write_in_full() semantics, where a partial write simply always returns a real error (and always sets 'errno' when that happens, including for the disk full case), a lot of the callers of write_in_full() were just unnecessarily complex. In particular, there's no reason to ever check for a zero length or return: if the length was zero, we'll return zero, otherwise, if a disk full resulted in the actual write() system call returning zero the write_in_full() logic would have correctly turned that into a negative return value, with 'errno' set to ENOSPC. I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Signed-off-by: Linus Torvalds --- I actually think "read_in_full()" should get the same loving tender care too, for all the same reasons. I think "read_or_die()" is totally broken. Anybody who uses "read_or_die()" is buggy by definition, since it will do a partial read AND NOT RETURN ANY INDICATION THAT IT WAS PARTIAL! Can I please ask people who do these idiotic cleanups to get their act together? I'll send a patch for that next, but I looked at "write_in_full()" first, for historical reasons. diff --git a/sha1_file.c b/sha1_file.c index 18dd89b..2a5be53 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1618,14 +1618,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename) static int write_buffer(int fd, const void *buf, size_t len) { - ssize_t size; - - if (!len) - return 0; - size = write_in_full(fd, buf, len); - if (!size) - return error("file write: disk full"); - if (size < 0) + if (write_in_full(fd, buf, len) < 0) return error("file write error (%s)", strerror(errno)); return 0; } diff --git a/write_or_die.c b/write_or_die.c index 488de72..1224cac 100644 --- a/write_or_die.c +++ b/write_or_die.c @@ -58,14 +58,7 @@ int write_in_full(int fd, const void *buf, size_t count) void write_or_die(int fd, const void *buf, size_t count) { - ssize_t written; - - if (!count) - return; - written = write_in_full(fd, buf, count); - if (written == 0) - die("disk full?"); - else if (written < 0) { + if (write_in_full(fd, buf, count) < 0) { if (errno == EPIPE) exit(0); die("write error (%s)", strerror(errno)); @@ -74,16 +67,7 @@ void write_or_die(int fd, const void *buf, size_t count) int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg) { - ssize_t written; - - if (!count) - return 1; - written = write_in_full(fd, buf, count); - if (written == 0) { - fprintf(stderr, "%s: disk full?\n", msg); - return 0; - } - else if (written < 0) { + if (write_in_full(fd, buf, count) < 0) { if (errno == EPIPE) exit(0); fprintf(stderr, "%s: write error (%s)\n", @@ -96,16 +80,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg) int write_or_whine(int fd, const void *buf, size_t count, const char *msg) { - ssize_t written; - - if (!count) - return 1; - written = write_in_full(fd, buf, count); - if (written == 0) { - fprintf(stderr, "%s: disk full?\n", msg); - return 0; - } - else if (written < 0) { + if (write_in_full(fd, buf, count) < 0) { fprintf(stderr, "%s: write error (%s)\n", msg, strerror(errno)); return 0; ``` ## Shawn O. Pearce, 2007-01-12 04:33 Subject: Re: Clean up write_in_full() users Message-ID: <20070112043346.GB24195@spearce.org> URL: https://gitlist.dev/e/20070112043346.GB24195%40spearce.org In-Reply-To: ``` Linus Torvalds wrote: > I actually think "read_in_full()" should get the same loving tender care > too, for all the same reasons. I think "read_or_die()" is totally broken. > Anybody who uses "read_or_die()" is buggy by definition, since it will do > a partial read AND NOT RETURN ANY INDICATION THAT IT WAS PARTIAL! AFAIK the only user of read_or_die is sha1_file.c when it reads in the 12 byte pack header and the 20 byte pack trailer to "quickly" verify the packfile is sane before using it. If I recall correctly it was correct when I created it, but the read_in_full refactoring changed it. -- Shawn. ``` ## Linus Torvalds, 2007-01-12 04:43 Subject: Re: Clean up write_in_full() users Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701112039100.3594%40woody.osdl.org In-Reply-To: <20070112043346.GB24195@spearce.org> ``` On Thu, 11 Jan 2007, Shawn O. Pearce wrote: > > AFAIK the only user of read_or_die is sha1_file.c when it reads in > the 12 byte pack header and the 20 byte pack trailer to "quickly" > verify the packfile is sane before using it. If I recall correctly > it was correct when I created it, but the read_in_full refactoring > changed it. Well, I just sent a patch to hopefully fix it, but I also think that the whole function is really misdesigned. The thing is, "write_or_die()" actually makes sense. If you get a write error, things are really seriously broken, and it makes a lot of sense to just say "things are totally broken". HOWEVER. The same thing is not true of a partial read. If a read doesn't succeed fully, the most _common_ case is that a file is truncated, and it does generally NOT make sense to just die and say "partial file". So for a write error, it's ok to say "ok, I can't write, I'm dead". For a read error, at the very least you have to say WHICH FILE couldn't be read, because it's usually a matter of some file just being too short, not some system-wide problem. Looking at the two call-sites of "read_or_die()", they really are better off not dying, or if they died, the caller should have passed in the _reason_ (ie the name of the pack-file), or should have just used a non-dying version and tested the return value and written a message of its own. But at least the function now WORKS after the patch I just sent out. Which it didn't do before. Linus ``` ## Morten Welinder, 2007-01-12 14:37 Subject: Re: Clean up write_in_full() users Message-ID: <118833cc0701120637i5808ac76g6ba4229cbe3aa26b@mail.gmail.com> URL: https://gitlist.dev/e/118833cc0701120637i5808ac76g6ba4229cbe3aa26b%40mail.gmail.com In-Reply-To: ``` > The thing is, "write_or_die()" actually makes sense. Except for a library. Calling exit is very unappealing in that case and trying to change die to use longjmp is even less appealing. M. ```