{"thread":{"id":"11563","subject":"[PATCH] bundle, fast-import: detect write failure","startedAt":"2008-01-10T08:54:25Z","lastAt":"2008-01-11T11:39:40Z","messageCount":13,"participants":["Jim Meyering","Pierre Habouzit","Johannes Schindelin","Junio C Hamano","Jakub Narebski","David Tweed"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"64909","messageId":"874pdmhxha.fsf@rho.meyering.net","threadId":"11563","inReplyTo":null,"subject":"[PATCH] bundle, fast-import: detect write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2008-01-10T08:54:25Z","receivedAt":"2008-01-10T08:54:25Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nI noticed some unchecked writes. This fixes them.\n\n* bundle.c (create_bundle): Die upon write failure.\n* fast-import.c (keep_pack): Die upon write or close failure.\n\nSigned-off-by: Jim Meyering \n---\n bundle.c | 6 +++---\n fast-import.c | 5 +++--\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex be204d8..316aa74 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -320,9 +320,9 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tfor (i = 0; i < revs.pending.nr; i++) {\n \t\tstruct object *object = revs.pending.objects[i].item;\n \t\tif (object->flags & UNINTERESTING)\n-\t\t\twrite(rls.in, \"^\", 1);\n-\t\twrite(rls.in, sha1_to_hex(object->sha1), 40);\n-\t\twrite(rls.in, \"\\n\", 1);\n+\t\t\twrite_or_die(rls.in, \"^\", 1);\n+\t\twrite_or_die(rls.in, sha1_to_hex(object->sha1), 40);\n+\t\twrite_or_die(rls.in, \"\\n\", 1);\n \t}\n \tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\ndiff --git a/fast-import.c b/fast-import.c\nindex 74597c9..82e9161 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -878,8 +878,9 @@ static char *keep_pack(char *curr_index_name)\n \tkeep_fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);\n \tif (keep_fd < 0)\n \t\tdie(\"cannot create keep file\");\n-\twrite(keep_fd, keep_msg, strlen(keep_msg));\n-\tclose(keep_fd);\n+\twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n+\tif (close(keep_fd))\n+\t\tdie(\"failed to write keep file\");\n\n \tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n \t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n--\n1.5.4.rc2.85.g71fd\n"},{"id":"64912","messageId":"20080110091733.GB17944@artemis.madism.org","threadId":"11563","inReplyTo":"874pdmhxha.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-01-10T09:17:33Z","receivedAt":"2008-01-10T09:17:33Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jan 10, 2008 at 08:54:25AM +0000, Jim Meyering wrote:\n> \n> I noticed some unchecked writes. This fixes them.\n\n Yeah, while we're at it, compiling git with -D_FORTIFY_SOURCE=2 isn't\nreally brilliant right now, there are quite many places with unchecked\nwrites, fwrites and chdirs.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"64921","messageId":"alpine.LSU.1.00.0801101204120.31053@racer.site","threadId":"11563","inReplyTo":"874pdmhxha.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-10T12:05:50Z","receivedAt":"2008-01-10T12:05:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jan 2008, Jim Meyering wrote:\n\n> I noticed some unchecked writes. This fixes them.\n\nThank you.\n\nHowever, you also have this:\n\n> -\tclose(keep_fd);\n> +\tif (close(keep_fd))\n> +\t\tdie(\"failed to write keep file\");\n\nI recently read an article which got me thinking about close(). The \nauthor maintained that many mistakes are done by being overzealously \ndefensive; die()ing in case of a close() failure (when open() succeeded!) \nmight be just wrong.\n\nCiao,\nDscho\n"},{"id":"64924","messageId":"87myrdhnn5.fsf@rho.meyering.net","threadId":"11563","inReplyTo":"alpine.LSU.1.00.0801101204120.31053@racer.site","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2008-01-10T12:26:54Z","receivedAt":"2008-01-10T12:26:54Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Johannes Schindelin wrote:\n> However, you also have this:\n>\n>> -\tclose(keep_fd);\n>> +\tif (close(keep_fd))\n>> +\t\tdie(\"failed to write keep file\");\n\nYes. I mentioned that in the commit log:\n\n * bundle.c (create_bundle): Die upon write failure.\n * fast-import.c (keep_pack): Die upon write or close failure.\n\nBut even the summary is accurate if you interpret\n\"write\" not as the syscall, but as the semantic\npush-data-through-OS-to-disk operation.\n\n> I recently read an article which got me thinking about close(). The\n> author maintained that many mistakes are done by being overzealously\n> defensive; die()ing in case of a close() failure (when open() succeeded!)\n> might be just wrong.\n\nNo. Whether open succeeded is a separate matter.\nAvoiding an unreported write (or close-writable-fd) failure is not\nbeing \"overzealously defensive.\"\n\n>From \"man 2 close\",\n\n -------------\n NOTES\n Not checking the return value of close() is a common but nevertheless\n serious programming error. It is quite possible that errors on a pre-\n vious write(2) operation are first reported at the final close(). Not\n checking the return value when closing the file may lead to silent loss\n of data. This can especially be observed with NFS and with disk quota.\n -------------\n"},{"id":"64925","messageId":"alpine.LSU.1.00.0801101234580.31053@racer.site","threadId":"11563","inReplyTo":"87myrdhnn5.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-10T12:37:56Z","receivedAt":"2008-01-10T12:37:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jan 2008, Jim Meyering wrote:\n\n> Johannes Schindelin wrote:\n>\n> > I recently read an article which got me thinking about close(). The \n> > author maintained that many mistakes are done by being overzealously \n> > defensive; die()ing in case of a close() failure (when open() \n> > succeeded!) might be just wrong.\n> \n> No. Whether open succeeded is a separate matter. Avoiding an unreported \n> write (or close-writable-fd) failure is not being \"overzealously \n> defensive.\"\n\nAre you aware what this code does? It writes a \".keep\" file. Whose \npurpose is to _exist_, and whose purpose is fulfilled, even if the write \nor the push-back did not succeed.\n\nI could not care less what the manual says. What is important is if the \ndefensive programming is done mindlessly, and therefore can fail so not \ngracefully.\n\nCiao,\nDscho\n"},{"id":"64926","messageId":"87hchlhm3k.fsf@rho.meyering.net","threadId":"11563","inReplyTo":"alpine.LSU.1.00.0801101234580.31053@racer.site","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2008-01-10T13:00:15Z","receivedAt":"2008-01-10T13:00:15Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Johannes Schindelin wrote:\n> Are you aware what this code does? It writes a \".keep\" file. Whose\n> purpose is to _exist_, and whose purpose is fulfilled, even if the write\n> or the push-back did not succeed.\n\nHi,\n\nI do see what you mean.\n\nIf the write is not necessary, then perhaps you would prefer a comment\ndocumenting that failures of the write and following close are ignorable.\nAnd add a '(void)' stmt prefix, to tell compilers that ignoring the\nreturn value is deliberate.\n\nHowever, even if it's not technically required to fail at that point,\nif it were my choice, I'd prefer to know when a .keep file whose\ncontents are unimportant just happens to reside on a bad spot on my\ndisk. I/O errors should never be ignored.\n\n> I could not care less what the manual says. What is important is if the\n> defensive programming is done mindlessly, and therefore can fail so not\n> gracefully.\n\nOn the other hand, if that write failure is truly ignorable,\na mindless minimalist :-) might argue that it's best just to\nomit the syscall.\n"},{"id":"64934","messageId":"20080110162526.GB27808@artemis.madism.org","threadId":"11563","inReplyTo":"87hchlhm3k.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Pierre Habouzit","fromEmail":"madcoder@artemis.madism.org","sentAt":"2008-01-10T16:25:26Z","receivedAt":"2008-01-10T16:25:26Z","isPatch":true,"sender":{"key":"madcoder@artemis.madism.org","avatar":null},"body":"On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:\n> Johannes Schindelin wrote:\n> > Are you aware what this code does? It writes a \".keep\" file. Whose\n> > purpose is to _exist_, and whose purpose is fulfilled, even if the write\n> > or the push-back did not succeed.\n> \n> Hi,\n> \n> I do see what you mean.\n> \n> If the write is not necessary, then perhaps you would prefer a comment\n> documenting that failures of the write and following close are ignorable.\n> And add a '(void)' stmt prefix, to tell compilers that ignoring the\n> return value is deliberate.\n\n Note that (void) isn't enough with the most recent gcc flavours, which\nis a pain. I do use:\n\n#define IGNORE(expr) do { if (expr) (void)0; } while (0)\n\nfor that purpose in my code. I know IGNORE isn't a brilliant name, but\nit's modeled after the ocaml function doing the same thing.\n\n> However, even if it's not technically required to fail at that point,\n> if it were my choice, I'd prefer to know when a .keep file whose\n> contents are unimportant just happens to reside on a bad spot on my\n> disk. I/O errors should never be ignored.\n\n Actually I think .keep files are empty, so the write() should not be\nthere in the first place, and we should only check for close() right ?\nnot that it matters that much.\n\n> > I could not care less what the manual says. What is important is if the\n> > defensive programming is done mindlessly, and therefore can fail so not\n> > gracefully.\n> \n> On the other hand, if that write failure is truly ignorable,\n> a mindless minimalist :-) might argue that it's best just to\n> omit the syscall.\n\n And leak a file descriptor :)\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"64936","messageId":"87lk6xftdn.fsf@rho.meyering.net","threadId":"11563","inReplyTo":"20080110162526.GB27808@artemis.madism.org","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2008-01-10T18:05:56Z","receivedAt":"2008-01-10T18:05:56Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Pierre Habouzit wrote:\n...\n>> On the other hand, if that write failure is truly ignorable,\n>> a mindless minimalist :-) might argue that it's best just to\n>> omit the syscall.\n>\n> And leak a file descriptor :)\n\nNot that mindless.\nThe *write* syscall, not the close.\nI would never suggest eliminating the close.\n"},{"id":"64937","messageId":"20080110181838.GC27808@artemis.madism.org","threadId":"11563","inReplyTo":"87lk6xftdn.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Pierre Habouzit","fromEmail":"madcoder@artemis.madism.org","sentAt":"2008-01-10T18:18:38Z","receivedAt":"2008-01-10T18:18:38Z","isPatch":true,"sender":{"key":"madcoder@artemis.madism.org","avatar":null},"body":"On Thu, Jan 10, 2008 at 06:05:56PM +0000, Jim Meyering wrote:\n> Pierre Habouzit wrote:\n> ....\n> >> On the other hand, if that write failure is truly ignorable,\n> >> a mindless minimalist :-) might argue that it's best just to\n> >> omit the syscall.\n> >\n> > And leak a file descriptor :)\n> \n> Not that mindless.\n> The *write* syscall, not the close.\n> I would never suggest eliminating the close.\n\n oh *oops*\n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"64989","messageId":"7vejco4xv5.fsf@gitster.siamese.dyndns.org","threadId":"11563","inReplyTo":"87hchlhm3k.fsf@rho.meyering.net","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-11T07:36:46Z","receivedAt":"2008-01-11T07:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering writes:\n\n> On the other hand, if that write failure is truly ignorable,\n> a mindless minimalist :-) might argue that it's best just to\n> omit the syscall.\n\nUsually the contents of .keep file is a small one-line comment\nthat describes who decided that the pack needs to be kept and\nwhy, so the answer is no.\n\nIn this case, a failure while closing that small .keep file is\nhighly unlikely, and if we ever mange to trigger such a highly\nunlikely failure, I think we would rather want to *know* about\nit, as it is likely there is something more seriously wrong\ngoing on.\n\nSo let's keep that check on close().\n"},{"id":"65001","messageId":"fm7c0p$n9a$1@ger.gmane.org","threadId":"11563","inReplyTo":"20080110162526.GB27808@artemis.madism.org","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-01-11T09:14:01Z","receivedAt":"2008-01-11T09:14:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Pierre Habouzit wrote:\n> On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:\n\n>> However, even if it's not technically required to fail at that point,\n>> if it were my choice, I'd prefer to know when a .keep file whose\n>> contents are unimportant just happens to reside on a bad spot on my\n>> disk. I/O errors should never be ignored.\n> \n> Actually I think .keep files are empty, so the write() should not be\n> there in the first place, and we should only check for close() right ?\n> not that it matters that much.\n\nIn theory the .keep file should contain description _why_ the pack\nis made kept. In practice git creates IIRC empty .kep files.\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"65002","messageId":"e1dab3980801110137o2440ccafxa4d3cc84630ce13b@mail.gmail.com","threadId":"11563","inReplyTo":"7vejco4xv5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"David Tweed","fromEmail":"david.tweed@gmail.com","sentAt":"2008-01-11T09:37:02Z","receivedAt":"2008-01-11T09:37:02Z","isPatch":true,"sender":{"key":"david.tweed@gmail.com","avatar":null},"body":"On Jan 11, 2008 7:36 AM, Junio C Hamano wrote:\n> In this case, a failure while closing that small .keep file is\n> highly unlikely, and if we ever mange to trigger such a highly\n> unlikely failure, I think we would rather want to *know* about\n> it, as it is likely there is something more seriously wrong\n> going on.\n\nOn a slightly related note: I've got a patch that handles the issue\nthat I reported a couple of months back that tmp pack/index objects\nwhere a write fails partway through are not deleted by any git\nprocessing, ie, when for example during git gc --prune we get\n\nfatal: sha1 file '/media/usbdiskc/v.git/objects/tmp_pack_QCYYAi' write\nerror (No space left on device)\nerror: failed to run repack\n\nbut the tmp_pack_* isn't deleted. I put my patch on the back burner\nwhen Junio declared a moratorium on new behaviours until after 1.5.4\ngets released, but will post once things open up again.\n\nAs it relates to this discussion: one of the awkward things is that\nthe die stuff doesn't leave any programatic indication (ie, not just a\nmessage to stderr) that a file is malformed due to a writing failure.\nPer Nicolas Pitre's suggestion to delete failed tmp_ files during a\n\"git gc --prune\", I just delete ALL tmp_ files at that time. This\napproach seems a bit risky -- can something like a git-svn fetch which\ngenerated tmp_ files by a different route be going on at the same time\nas a git gc? -- but I couldn't think of another way to do it.\n\n-- \ncheers, dave tweed__________________________\ndavid.tweed@gmail.com\nRm 124, School of Systems Engineering, University of Reading.\n\"we had no idea that when we added templates we were adding a Turing-\ncomplete compile-time language.\" -- C++ standardisation committee\n"},{"id":"65007","messageId":"Pine.LNX.4.64.0801111237540.14355@wbgn129.biozentrum.uni-wuerzburg.de","threadId":"11563","inReplyTo":"7vejco4xv5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bundle, fast-import: detect write failure","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-11T11:39:40Z","receivedAt":"2008-01-11T11:39:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jan 2008, Junio C Hamano wrote:\n\n> In this case, a failure while closing that small .keep file is highly \n> unlikely, and if we ever mange to trigger such a highly unlikely \n> failure, I think we would rather want to *know* about it, as it is \n> likely there is something more seriously wrong going on.\n> \n> So let's keep that check on close().\n\nMy comment was not about that _check_, but about having this die() instead \nof just printing out a warning.\n\nIf that close fails, strange things are going on, alright. But neither \nthe open() nor the write() failed at that point, so IMO it would be a \nmistake to error out _here_. If later stages fail also, well, we can \ndie() there, no?\n\nCiao,\nDscho\n"}]}