{"thread":{"id":"6347","subject":"Clean up write_in_full() users","startedAt":"2007-01-12T04:23:00Z","lastAt":"2007-01-12T14:37:46Z","messageCount":4,"participants":["Linus Torvalds","Shawn O. Pearce","Morten Welinder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"31540","messageId":"Pine.LNX.4.64.0701112014050.3594@woody.osdl.org","threadId":"6347","inReplyTo":null,"subject":"Clean up write_in_full() users","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-12T04:23:00Z","receivedAt":"2007-01-12T04:23:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWith the new-and-improved write_in_full() semantics, where a partial write \nsimply always returns a real error (and always sets 'errno' when that \nhappens, including for the disk full case), a lot of the callers of \nwrite_in_full() were just unnecessarily complex.\n\nIn particular, there's no reason to ever check for a zero length or \nreturn: if the length was zero, we'll return zero, otherwise, if a disk \nfull resulted in the actual write() system call returning zero the \nwrite_in_full() logic would have correctly turned that into a negative \nreturn value, with 'errno' set to ENOSPC.\n\nI really wish every \"write_in_full()\" user would just check against \"<0\" \nnow, but this fixes the nasty and stupid ones.\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\n\nI actually think \"read_in_full()\" should get the same loving tender care \ntoo, for all the same reasons. I think \"read_or_die()\" is totally broken. \nAnybody who uses \"read_or_die()\" is buggy by definition, since it will do \na partial read AND NOT RETURN ANY INDICATION THAT IT WAS PARTIAL!\n\nCan I please ask people who do these idiotic cleanups to get their act \ntogether?\n\nI'll send a patch for that next, but I looked at \"write_in_full()\" first, \nfor historical reasons.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 18dd89b..2a5be53 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1618,14 +1618,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \n static int write_buffer(int fd, const void *buf, size_t len)\n {\n-\tssize_t size;\n-\n-\tif (!len)\n-\t\treturn 0;\n-\tsize = write_in_full(fd, buf, len);\n-\tif (!size)\n-\t\treturn error(\"file write: disk full\");\n-\tif (size < 0)\n+\tif (write_in_full(fd, buf, len) < 0)\n \t\treturn error(\"file write error (%s)\", strerror(errno));\n \treturn 0;\n }\ndiff --git a/write_or_die.c b/write_or_die.c\nindex 488de72..1224cac 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -58,14 +58,7 @@ int write_in_full(int fd, const void *buf, size_t count)\n \n void write_or_die(int fd, const void *buf, size_t count)\n {\n-\tssize_t written;\n-\n-\tif (!count)\n-\t\treturn;\n-\twritten = write_in_full(fd, buf, count);\n-\tif (written == 0)\n-\t\tdie(\"disk full?\");\n-\telse if (written < 0) {\n+\tif (write_in_full(fd, buf, count) < 0) {\n \t\tif (errno == EPIPE)\n \t\t\texit(0);\n \t\tdie(\"write error (%s)\", strerror(errno));\n@@ -74,16 +67,7 @@ void write_or_die(int fd, const void *buf, size_t count)\n \n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n-\tssize_t written;\n-\n-\tif (!count)\n-\t\treturn 1;\n-\twritten = write_in_full(fd, buf, count);\n-\tif (written == 0) {\n-\t\tfprintf(stderr, \"%s: disk full?\\n\", msg);\n-\t\treturn 0;\n-\t}\n-\telse if (written < 0) {\n+\tif (write_in_full(fd, buf, count) < 0) {\n \t\tif (errno == EPIPE)\n \t\t\texit(0);\n \t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n@@ -96,16 +80,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n \n int write_or_whine(int fd, const void *buf, size_t count, const char *msg)\n {\n-\tssize_t written;\n-\n-\tif (!count)\n-\t\treturn 1;\n-\twritten = write_in_full(fd, buf, count);\n-\tif (written == 0) {\n-\t\tfprintf(stderr, \"%s: disk full?\\n\", msg);\n-\t\treturn 0;\n-\t}\n-\telse if (written < 0) {\n+\tif (write_in_full(fd, buf, count) < 0) {\n \t\tfprintf(stderr, \"%s: write error (%s)\\n\",\n \t\t\tmsg, strerror(errno));\n \t\treturn 0;\n"},{"id":"31541","messageId":"20070112043346.GB24195@spearce.org","threadId":"6347","inReplyTo":"Pine.LNX.4.64.0701112014050.3594@woody.osdl.org","subject":"Re: Clean up write_in_full() users","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-12T04:33:46Z","receivedAt":"2007-01-12T04:33:46Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> wrote:\n> I actually think \"read_in_full()\" should get the same loving tender care \n> too, for all the same reasons. I think \"read_or_die()\" is totally broken. \n> Anybody who uses \"read_or_die()\" is buggy by definition, since it will do \n> a partial read AND NOT RETURN ANY INDICATION THAT IT WAS PARTIAL!\n\nAFAIK the only user of read_or_die is sha1_file.c when it reads in\nthe 12 byte pack header and the 20 byte pack trailer to \"quickly\"\nverify the packfile is sane before using it.  If I recall correctly\nit was correct when I created it, but the read_in_full refactoring\nchanged it.\n\n-- \nShawn.\n"},{"id":"31543","messageId":"Pine.LNX.4.64.0701112039100.3594@woody.osdl.org","threadId":"6347","inReplyTo":"20070112043346.GB24195@spearce.org","subject":"Re: Clean up write_in_full() users","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2007-01-12T04:43:50Z","receivedAt":"2007-01-12T04:43:50Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 Jan 2007, Shawn O. Pearce wrote:\n> \n> AFAIK the only user of read_or_die is sha1_file.c when it reads in\n> the 12 byte pack header and the 20 byte pack trailer to \"quickly\"\n> verify the packfile is sane before using it.  If I recall correctly\n> it was correct when I created it, but the read_in_full refactoring\n> changed it.\n\nWell, I just sent a patch to hopefully fix it, but I also think that the \nwhole function is really misdesigned.\n\nThe thing is, \"write_or_die()\" actually makes sense. If you get a write \nerror, things are really seriously broken, and it makes a lot of sense to \njust say \"things are totally broken\".\n\nHOWEVER. The same thing is not true of a partial read. If a read doesn't \nsucceed fully, the most _common_ case is that a file is truncated, and it \ndoes generally NOT make sense to just die and say \"partial file\".\n\nSo for a write error, it's ok to say \"ok, I can't write, I'm dead\". \n\nFor a read error, at the very least you have to say WHICH FILE couldn't be \nread, because it's usually a matter of some file just being too short, not \nsome system-wide problem.\n\nLooking at the two call-sites of \"read_or_die()\", they really are better \noff not dying, or if they died, the caller should have passed in the \n_reason_ (ie the name of the pack-file), or should have just used a \nnon-dying version and tested the return value and written a message of its \nown.\n\nBut at least the function now WORKS after the patch I just sent out. Which \nit didn't do before. \n\n\t\t\tLinus\n"},{"id":"31556","messageId":"118833cc0701120637i5808ac76g6ba4229cbe3aa26b@mail.gmail.com","threadId":"6347","inReplyTo":"Pine.LNX.4.64.0701112039100.3594@woody.osdl.org","subject":"Re: Clean up write_in_full() users","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2007-01-12T14:37:46Z","receivedAt":"2007-01-12T14:37:46Z","isPatch":false,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> The thing is, \"write_or_die()\" actually makes sense.\n\nExcept for a library.  Calling exit is very unappealing in that case and trying\nto change die to use longjmp is even less appealing.\n\nM.\n"}]}