{"thread":{"id":"45120","subject":"Confusing git messages when disk is full.","startedAt":"2017-02-12T16:37:36Z","lastAt":"2017-02-18T00:00:35Z","messageCount":24,"participants":["Jáchym Barvínek","Jeff King","Junio C Hamano","Andreas Schwab","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"311361","messageId":"CABpqov=FE-h_2s=O9fkSjFjgFXSy6hDwc2fu5ijiVvkaLx9f_Q@mail.gmail.com","threadId":"45120","inReplyTo":null,"subject":"Confusing git messages when disk is full.","fromName":"Jáchym Barvínek","fromEmail":"jachymb@gmail.com","sentAt":"2017-02-12T16:37:30Z","receivedAt":"2017-02-12T16:37:36Z","isPatch":false,"sender":{"key":"jachymb@gmail.com","avatar":null},"body":"Hello, I would like to report what I consider a bug in git, I hope I'm\ndoing it the right way.\nI was trying to run `git pull` in my repository and got the following\nerror: \"git pull\nYour configuration specifies to merge with the ref 'refs/heads/master'\nfrom the remote, but no such ref was fetched.\"\nWhich was very confusing to me, I found some answers to what might be the cause\nbut none was the right one. The actual cause was that the filesystem\nhad no more free space.\nWhen I cleaned the space, `git pull` then gave the expected answer\n(\"Already up-to-date.\").\nI think the message is confusing and git should be able to report to\nthe user that the cause\nis full disk.\nRegards, Jáchym Barvínek\n"},{"id":"311678","messageId":"20170215213221.lnraiktneokpk3mg@sigill.intra.peff.net","threadId":"45120","inReplyTo":"CABpqov=FE-h_2s=O9fkSjFjgFXSy6hDwc2fu5ijiVvkaLx9f_Q@mail.gmail.com","subject":"Re: Confusing git messages when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T21:32:22Z","receivedAt":"2017-02-15T21:32:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:\n\n> Hello, I would like to report what I consider a bug in git, I hope I'm\n> doing it the right way.\n> I was trying to run `git pull` in my repository and got the following\n> error: \"git pull\n> Your configuration specifies to merge with the ref 'refs/heads/master'\n> from the remote, but no such ref was fetched.\"\n\nIt sounds like writing FETCH_HEAD failed, and git-pull became\nconfused that the ref wasn't fetched.\n\n> Which was very confusing to me, I found some answers to what might be the cause\n> but none was the right one. The actual cause was that the filesystem\n> had no more free space.\n> When I cleaned the space, `git pull` then gave the expected answer\n> (\"Already up-to-date.\").\n> I think the message is confusing and git should be able to report to\n> the user that the cause\n> is full disk.\n\nIf FETCH_HEAD failed to write because of a full disk (or any other\nreason), then the right thing is for \"git fetch\" to write an error to\nstderr, and git-pull should not continue the operation at all.\n\nIf we're not doing that, then that is certainly a bug.\n\n-Peff\n"},{"id":"311680","messageId":"xmqqk28r2kk4.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170215213221.lnraiktneokpk3mg@sigill.intra.peff.net","subject":"Re: Confusing git messages when disk is full.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T21:47:23Z","receivedAt":"2017-02-15T21:47:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:\n> If FETCH_HEAD failed to write because of a full disk (or any other\n> reason), then the right thing is for \"git fetch\" to write an error to\n> stderr, and git-pull should not continue the operation at all.\n>\n> If we're not doing that, then that is certainly a bug.\n\nOne suspect would be store_updated_refs(). We do catch failure from\nfopen(\"a\") of FETCH_HEAD (it is truncated earlier in the code when\nthe --append option is not given), but all the writes go through\nstdio without error checking.\n\nI wonder if this lazy patch is sufficient. I want to avoid having\nto sprinkle\n\n\tif (fputs(\"\\\\n\", fp))\n\t\terror(...);\n\nall over the code.\n\n builtin/fetch.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b5ad09d046..72347f0054 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n abort:\n \tstrbuf_release(¬e);\n \tfree(url);\n-\tfclose(fp);\n+\tif (ferror(fp))\n+\t\trc = -1;\n+\tif (fclose(fp))\n+\t\trc = -1;\n \treturn rc;\n }\n \n"},{"id":"311683","messageId":"20170215215151.a5chtxyjhbe3og4p@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqqk28r2kk4.fsf@gitster.mtv.corp.google.com","subject":"Re: Confusing git messages when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T21:51:51Z","receivedAt":"2017-02-15T21:51:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 01:47:23PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:\n> > If FETCH_HEAD failed to write because of a full disk (or any other\n> > reason), then the right thing is for \"git fetch\" to write an error to\n> > stderr, and git-pull should not continue the operation at all.\n> >\n> > If we're not doing that, then that is certainly a bug.\n> \n> One suspect would be store_updated_refs(). We do catch failure from\n> fopen(\"a\") of FETCH_HEAD (it is truncated earlier in the code when\n> the --append option is not given), but all the writes go through\n> stdio without error checking.\n> \n> I wonder if this lazy patch is sufficient. I want to avoid having\n> to sprinkle\n> \n> \tif (fputs(\"\\\\n\", fp))\n> \t\terror(...);\n> \n> all over the code.\n\nHeh, I was just tracking down the exact same spot.\n\nI think that yes, the lazy check-error-flag-at-the-end approach is fine\nfor stdio.\n\nI tried to reproduce the original problem on a full loopback filesystem,\nbut got:\n\n fatal: update_ref failed for ref 'ORIG_HEAD': could not write to '.git/ORIG_HEAD'\n\nI suspect you'd need the _exact_ right amount of free space to get all\nof the predecessor steps done, and then run out of space right when\ntrying to flush the FETCH_HEAD contents.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index b5ad09d046..72347f0054 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n> abort:\n> \tstrbuf_release(¬e);\n> \tfree(url);\n> -\tfclose(fp);\n> +\tif (ferror(fp))\n> +\t\trc = -1;\n> +\tif (fclose(fp))\n> +\t\trc = -1;\n> \treturn rc;\n\nYeah, I think this works. Normally you'd want to flush before checking\nferror(), but since you detect errors from fclose, too, it should be\nfine.\n\nWe probably should write something stderr, though. Maybe:\n\n if (ferror(fp) || fclose(fp))\n\trc = error_errno(\"unable to write to %s\", filename);\n\n-Peff\n"},{"id":"311689","messageId":"xmqq7f4r2io5.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170215215151.a5chtxyjhbe3og4p@sigill.intra.peff.net","subject":"Re: Confusing git messages when disk is full.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T22:28:10Z","receivedAt":"2017-02-15T22:28:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> abort:\n>> \tstrbuf_release(¬e);\n>> \tfree(url);\n>> -\tfclose(fp);\n>> +\tif (ferror(fp))\n>> +\t\trc = -1;\n>> +\tif (fclose(fp))\n>> +\t\trc = -1;\n>> \treturn rc;\n>\n> Yeah, I think this works. Normally you'd want to flush before checking\n> ferror(), but since you detect errors from fclose, too, it should be\n> fine.\n>\n> We probably should write something stderr, though. Maybe:\n>\n> if (ferror(fp) || fclose(fp))\n> \trc = error_errno(\"unable to write to %s\", filename);\n\nYes, and somehow make sure we do fclose(fp) even when we have an\nerror already ;-)\n"},{"id":"311690","messageId":"20170215223246.mkaz22yrovnscnne@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqq7f4r2io5.fsf@gitster.mtv.corp.google.com","subject":"Re: Confusing git messages when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T22:32:46Z","receivedAt":"2017-02-15T22:32:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 02:28:10PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> abort:\n> >> \tstrbuf_release(¬e);\n> >> \tfree(url);\n> >> -\tfclose(fp);\n> >> +\tif (ferror(fp))\n> >> +\t\trc = -1;\n> >> +\tif (fclose(fp))\n> >> +\t\trc = -1;\n> >> \treturn rc;\n> >\n> > Yeah, I think this works. Normally you'd want to flush before checking\n> > ferror(), but since you detect errors from fclose, too, it should be\n> > fine.\n> >\n> > We probably should write something stderr, though. Maybe:\n> >\n> > if (ferror(fp) || fclose(fp))\n> > \trc = error_errno(\"unable to write to %s\", filename);\n> \n> Yes, and somehow make sure we do fclose(fp) even when we have an\n> error already ;-)\n\nGood catch. I think we use a nasty bitwise-OR elsewhere to do that.\nAh, here it is, in tempfile.c:\n\n /*\n * Note: no short-circuiting here; we want to fclose()\n * in any case!\n */\n err = ferror(fp) | fclose(fp);\n\nThat works, but the fact that we need a comment is a good sign that it's\nkind of gross. It's too bad stdio does not specify the return of fclose\nto report an error in the close _or_ any previous error. I guess we\ncould wrap it with our own function.\n\n-Peff\n"},{"id":"311693","messageId":"xmqq37ff2hn8.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170215223246.mkaz22yrovnscnne@sigill.intra.peff.net","subject":"Re: Confusing git messages when disk is full.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-15T22:50:19Z","receivedAt":"2017-02-15T22:50:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Good catch. I think we use a nasty bitwise-OR elsewhere to do that.\n> Ah, here it is, in tempfile.c:\n>\n> /*\n> * Note: no short-circuiting here; we want to fclose()\n> * in any case!\n> */\n> err = ferror(fp) | fclose(fp);\n>\n> That works, but the fact that we need a comment is a good sign that it's\n> kind of gross. It's too bad stdio does not specify the return of fclose\n> to report an error in the close _or_ any previous error. I guess we\n> could wrap it with our own function.\n\nSure. I am happy to add something like this:\n\n\t/*\n\t * closes a FILE *, returns 0 if closing and all the\n\t * previous stdio operations on fp were successful,\n\t * otherwise non-zero.\n\t */\n\tint xfclose(FILE *fp)\n\t{\n\t\treturn ferror(fp) | fclose(fp);\n\t}\n\nI do not think we should try to do anything fancier to allow the\ncaller to tell ferror() and fclose() apart, as such a caller would\nthen need to do\n\n\tswitch (xfclose(fp)) {\n\tcase 0: /* happy */ break;\n\tcase XFCLOSE_CLOSE: do \"close failed\" thing; break;\n\tcase XFCLOSE_ERROR: do \"other things failed\" thing; break;\n\t}\n\nand at that point, \"other things failed\" code would not have much to\nwork with to do more detailed diagnosis anyway (the errno is likely\nnot trustable), and it is not too much to write\n\n\tif (ferror(fp))\n\t\tdo \"saw some failure before\" thing;\n\tif (fclose(fp))\n\t\tdo \"close failed\" thing;\n\ninstead.\n \n"},{"id":"311709","messageId":"20170215231832.bzg3ygz4ualcvqlc@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqq37ff2hn8.fsf@gitster.mtv.corp.google.com","subject":"Re: Confusing git messages when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-15T23:18:33Z","receivedAt":"2017-02-16T00:00:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:\n\n> > That works, but the fact that we need a comment is a good sign that it's\n> > kind of gross. It's too bad stdio does not specify the return of fclose\n> > to report an error in the close _or_ any previous error. I guess we\n> > could wrap it with our own function.\n> \n> Sure. I am happy to add something like this:\n> \n> \t/*\n> \t * closes a FILE *, returns 0 if closing and all the\n> \t * previous stdio operations on fp were successful,\n> \t * otherwise non-zero.\n> \t */\n> \tint xfclose(FILE *fp)\n> \t{\n> \t\treturn ferror(fp) | fclose(fp);\n> \t}\n\nYes, that's exactly what I had in mind (might be worth calling out the\nbitwise-OR, though, just to make it clear it's not a typo).\n\n> I do not think we should try to do anything fancier to allow the\n> caller to tell ferror() and fclose() apart, as such a caller would\n> then need to do\n\nAbsolutely. If they care, they can call the two separately.\n\nYou are right that errno is untrustworthy in the ferror() case, though.\nMaybe that is reason not to add xfclose, and just force this caller to\ndo something like:\n\n if (ferror(fp))\n\trc = error(\"unable to write to %s\", filename);\n if (fclose(fp))\n\trc = error_errno(\"unable to write to %s\", filename);\n\nOf course, if the earlier error persists through fclose, we'd print two\nerrors. This would all be much easier if the filehandles kept not just\nan error bit, but the original errno. \n\nMaybe a not-terrible compromise is to fake the errno in the ferror case,\nlike:\n\n int xfclose(FILE *fp)\n {\n\tint error_flag = ferror(fp);\n\n\t/*\n\t * If we get an error from fclose, the current errno value is\n\t * trustworthy. But if it succeeds and we had a previous error,\n\t * we need to report failure, but the value of errno could\n\t * be unrelated. Make up a generic errno value.\n\t */\n\tif (fclose(fp))\n\t\treturn EOF;\n\t} else if (error_flag) {\n\t\terrno = EINVAL; /* or EIO? */\n\t\treturn EOF;\n\t} else {\n\t\treturn 0;\n\t}\n }\n\nOr maybe that would just confuse people when they later get \"invalid\nargument\" in the error message. I wish there was an errno value for \"I\ndon't remember what the error was\".\n\nI dunno. This whole thing is ending up a lot more complicated than I had\nhoped. I just didn't want to have to say \"unable to write to %s\" twice. ;)\n\n-Peff\n"},{"id":"311732","messageId":"87tw7uv439.fsf@linux-m68k.org","threadId":"45120","inReplyTo":"20170215231832.bzg3ygz4ualcvqlc@sigill.intra.peff.net","subject":"Re: Confusing git messages when disk is full.","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-02-16T10:10:18Z","receivedAt":"2017-02-16T10:10:30Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Feb 15 2017, Jeff King wrote:\n\n> On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:\n>\n>> > That works, but the fact that we need a comment is a good sign that it's\n>> > kind of gross. It's too bad stdio does not specify the return of fclose\n>> > to report an error in the close _or_ any previous error. I guess we\n>> > could wrap it with our own function.\n>> \n>> Sure. I am happy to add something like this:\n>> \n>> \t/*\n>> \t * closes a FILE *, returns 0 if closing and all the\n>> \t * previous stdio operations on fp were successful,\n>> \t * otherwise non-zero.\n>> \t */\n>> \tint xfclose(FILE *fp)\n>> \t{\n>> \t\treturn ferror(fp) | fclose(fp);\n>> \t}\n>\n> Yes, that's exactly what I had in mind (might be worth calling out the\n> bitwise-OR, though, just to make it clear it's not a typo).\n\nSince the order of evaluation is unspecified, it would be better to\nforce sequencing ferror before fclose.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"311769","messageId":"20170216164359.k2ab7laqqvusfsm2@sigill.intra.peff.net","threadId":"45120","inReplyTo":"87tw7uv439.fsf@linux-m68k.org","subject":"Re: Confusing git messages when disk is full.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T16:44:00Z","receivedAt":"2017-02-16T16:44:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:\n\n> >> \tint xfclose(FILE *fp)\n> >> \t{\n> >> \t\treturn ferror(fp) | fclose(fp);\n> >> \t}\n> >\n> > Yes, that's exactly what I had in mind (might be worth calling out the\n> > bitwise-OR, though, just to make it clear it's not a typo).\n> \n> Since the order of evaluation is unspecified, it would be better to\n> force sequencing ferror before fclose.\n\nGood point. Arguably the call in tempfile.c is buggy.\n\n-Peff\n"},{"id":"311812","messageId":"20170216213140.xqw7gzjimhvg7tcm@sigill.intra.peff.net","threadId":"45120","inReplyTo":"20170216164359.k2ab7laqqvusfsm2@sigill.intra.peff.net","subject":"[PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-16T21:31:40Z","receivedAt":"2017-02-16T21:31:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:\n\n> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:\n> \n> > >> \tint xfclose(FILE *fp)\n> > >> \t{\n> > >> \t\treturn ferror(fp) | fclose(fp);\n> > >> \t}\n> > >\n> > > Yes, that's exactly what I had in mind (might be worth calling out the\n> > > bitwise-OR, though, just to make it clear it's not a typo).\n> > \n> > Since the order of evaluation is unspecified, it would be better to\n> > force sequencing ferror before fclose.\n> \n> Good point. Arguably the call in tempfile.c is buggy.\n\nHere's a fix.\n\nI think close_tempfile() suffers from the same errno problem discussed\nearlier in this thread (i.e., that after calling it, you may get an\nerror return with a random, unrelated errno value if ferror() failed but\nfclose() did not).\n\n-- >8 --\nSubject: [PATCH] tempfile: avoid \"ferror | fclose\" trick\n\nThe current code wants to record an error condition from\neither ferror() or fclose(), but makes sure that we always\ncall both functions. So it can't use logical-OR \"||\", which\nwould short-circuit when ferror() is true. Instead, it uses\nbitwise-OR \"|\" to evaluate both functions and set one or\nmore bits in the \"err\" flag if they reported a failure.\n\nUnlike logical-OR, though, bitwise-OR does not introduce a\nsequence point, and the order of evaluation for its operands\nis unspecified. So a compiler would be free to generate code\nwhich calls fclose() first, and then ferror() on the\nnow-freed filehandle.\n\nThere's no indication that this has happened in practice,\nbut let's write it out in a way that follows the standard.\n\nNoticed-by: Andreas Schwab \nSigned-off-by: Jeff King \n---\n tempfile.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex 2990c9242..ffcc27237 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)\n \ttempfile->fd = -1;\n \tif (fp) {\n \t\ttempfile->fp = NULL;\n-\n-\t\t/*\n-\t\t * Note: no short-circuiting here; we want to fclose()\n-\t\t * in any case!\n-\t\t */\n-\t\terr = ferror(fp) | fclose(fp);\n+\t\terr = ferror(fp);\n+\t\terr |= fclose(fp);\n \t} else {\n \t\terr = close(fd);\n \t}\n-- \n2.12.0.rc1.559.gd292418bf\n\n"},{"id":"311843","messageId":"923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu","threadId":"45120","inReplyTo":"20170216213140.xqw7gzjimhvg7tcm@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-17T08:00:09Z","receivedAt":"2017-02-17T08:00:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/16/2017 10:31 PM, Jeff King wrote:\n> On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:\n> \n>> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:\n>>\n>>>>> \tint xfclose(FILE *fp)\n>>>>> \t{\n>>>>> \t\treturn ferror(fp) | fclose(fp);\n>>>>> \t}\n>>>>\n>>>> Yes, that's exactly what I had in mind (might be worth calling out the\n>>>> bitwise-OR, though, just to make it clear it's not a typo).\n>>>\n>>> Since the order of evaluation is unspecified, it would be better to\n>>> force sequencing ferror before fclose.\n>>\n>> Good point. Arguably the call in tempfile.c is buggy.\n> \n> Here's a fix.\n> \n> I think close_tempfile() suffers from the same errno problem discussed\n> earlier in this thread (i.e., that after calling it, you may get an\n> error return with a random, unrelated errno value if ferror() failed but\n> fclose() did not).\n> \n> -- >8 --\n> Subject: [PATCH] tempfile: avoid \"ferror | fclose\" trick\n> \n> The current code wants to record an error condition from\n> either ferror() or fclose(), but makes sure that we always\n> call both functions. So it can't use logical-OR \"||\", which\n> would short-circuit when ferror() is true. Instead, it uses\n> bitwise-OR \"|\" to evaluate both functions and set one or\n> more bits in the \"err\" flag if they reported a failure.\n> \n> Unlike logical-OR, though, bitwise-OR does not introduce a\n> sequence point, and the order of evaluation for its operands\n> is unspecified. So a compiler would be free to generate code\n> which calls fclose() first, and then ferror() on the\n> now-freed filehandle.\n> \n> There's no indication that this has happened in practice,\n> but let's write it out in a way that follows the standard.\n> \n> Noticed-by: Andreas Schwab \n> Signed-off-by: Jeff King \n> ---\n> tempfile.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/tempfile.c b/tempfile.c\n> index 2990c9242..ffcc27237 100644\n> --- a/tempfile.c\n> +++ b/tempfile.c\n> @@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)\n> \ttempfile->fd = -1;\n> \tif (fp) {\n> \t\ttempfile->fp = NULL;\n> -\n> -\t\t/*\n> -\t\t * Note: no short-circuiting here; we want to fclose()\n> -\t\t * in any case!\n> -\t\t */\n> -\t\terr = ferror(fp) | fclose(fp);\n> +\t\terr = ferror(fp);\n> +\t\terr |= fclose(fp);\n> \t} else {\n> \t\terr = close(fd);\n> \t}\n> \n\nThanks for fixing this; the old code was definitely wrong.\n\nAs you pointed out, if ferror() fails, it doesn't set errno properly. At\nleast one caller tries to strerror(errno), so it would probably be good\nto store *something* in there, probably EIO.\n\nTo be really pedantic, there's also the question of what errno the\ncaller would want if *both* ferror() and fclose() fail. Normally I would\nsay \"the first error that occurred\", but in this case we don't know the\ncorrect errno from the error reported by ferror(), so maybe the fclose()\nerrno is more likely to hint at the underlying reason for the failure.\n\nSo I (reluctantly) propose\n\n\tif (ferror(fp)) {\n\t\tif (!fclose(fp)) {\n\t\t\t/*\n\t\t\t * ferror() doesn't set errno, so we have to\n\t\t\t * set one. (By contrast, when fclose() fails\n\t\t\t * too, we leave *its* errno in place.)\n\t\t\t */\n\t\t\terrno = EIO;\n\t\t}\n\t\treturn -1;\n\t}\n\treturn fclose();\n\nMichael\n\n"},{"id":"311844","messageId":"20170217080759.2357wzdiuymcyosw@sigill.intra.peff.net","threadId":"45120","inReplyTo":"923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T08:07:59Z","receivedAt":"2017-02-17T08:08:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:\n\n> As you pointed out, if ferror() fails, it doesn't set errno properly. At\n> least one caller tries to strerror(errno), so it would probably be good\n> to store *something* in there, probably EIO.\n\nYeah, we discussed this up-thread a bit, and my \"solution\" was similar\nto yours. I don't like it, because EIO is a real thing that can happen,\ntoo, and it would certainly be surprising to a user to see. But it's\nprobably better than the alternative, which is getting whatever random\nvalue happened to be in errno.\n\nThe only downside is that if the value of errno _was_ valid (because the\nlast thing you did really was writing to the filehandle, then we'd\noverwrite it).\n\n> To be really pedantic, there's also the question of what errno the\n> caller would want if *both* ferror() and fclose() fail. Normally I would\n> say \"the first error that occurred\", but in this case we don't know the\n> correct errno from the error reported by ferror(), so maybe the fclose()\n> errno is more likely to hint at the underlying reason for the failure.\n\nYes, I think we're better to take what fclose gives us, if we can.\n\n> So I (reluctantly) propose\n> \n> \tif (ferror(fp)) {\n> \t\tif (!fclose(fp)) {\n> \t\t\t/*\n> \t\t\t * ferror() doesn't set errno, so we have to\n> \t\t\t * set one. (By contrast, when fclose() fails\n> \t\t\t * too, we leave *its* errno in place.)\n> \t\t\t */\n> \t\t\terrno = EIO;\n> \t\t}\n> \t\treturn -1;\n> \t}\n> \treturn fclose();\n\nThat's similar to what I wrote earlier, but if we don't mind overwriting\nerrno unconditionally, I think just:\n\n errno = EIO; /* covers ferror(), overwritten by failing fclose() */\n err |= ferror(fp);\n err |= fclose(fp);\n\ndoes the same thing.\n\n-Peff\n"},{"id":"311856","messageId":"64eedabd-c0de-a7e0-8d98-ad23a9625b45@alum.mit.edu","threadId":"45120","inReplyTo":"20170217080759.2357wzdiuymcyosw@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-02-17T10:42:25Z","receivedAt":"2017-02-17T10:42:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/17/2017 09:07 AM, Jeff King wrote:\n> [...]\n> That's similar to what I wrote earlier, but if we don't mind overwriting\n> errno unconditionally, I think just:\n> \n> errno = EIO; /* covers ferror(), overwritten by failing fclose() */\n> err |= ferror(fp);\n> err |= fclose(fp);\n> \n> does the same thing.\n\nTrue; I'd forgotten the convention that non-failing functions are\nallowed to change errno. Your solution is obviously simpler and faster.\n\nMichael\n\n"},{"id":"311935","messageId":"20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net","threadId":"45120","inReplyTo":"64eedabd-c0de-a7e0-8d98-ad23a9625b45@alum.mit.edu","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T20:54:42Z","receivedAt":"2017-02-17T20:54:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:\n\n> On 02/17/2017 09:07 AM, Jeff King wrote:\n> > [...]\n> > That's similar to what I wrote earlier, but if we don't mind overwriting\n> > errno unconditionally, I think just:\n> > \n> > errno = EIO; /* covers ferror(), overwritten by failing fclose() */\n> > err |= ferror(fp);\n> > err |= fclose(fp);\n> > \n> > does the same thing.\n> \n> True; I'd forgotten the convention that non-failing functions are\n> allowed to change errno. Your solution is obviously simpler and faster.\n\nI guess we are simultaneously assuming that it is OK to munge errno on\nsuccess in our function, but that fclose() will not do so. Which seems a\nbit hypocritical. Maybe the \"if\" dance is better.\n\n-Peff\n"},{"id":"311936","messageId":"20170217210749.6dqmaotleefid76y@sigill.intra.peff.net","threadId":"45120","inReplyTo":"20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T21:07:49Z","receivedAt":"2017-02-17T21:07:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 03:54:42PM -0500, Jeff King wrote:\n\n> I guess we are simultaneously assuming that it is OK to munge errno on\n> success in our function, but that fclose() will not do so. Which seems a\n> bit hypocritical. Maybe the \"if\" dance is better.\n\nSo here's that patch with a justification.\n\nAt this point, this snippet of code would be appropriate to pull into\nxfclose() if we wanted. But possibly that is the wrong direction, as it\nencourages callers to do:\n\n if (xfclose(fp))\n\terr = error_errno(\"failure writing to ...\");\n\nwhen they could do:\n\n if (ferror(fp))\n\terr = error(\"failure writing to ...\");\n if (fclose(fp))\n err = error_errno(\"failure writing to ...\");\n\nWhile longer, it's arguably better for them to distinguish the two\ncases. It's only worth doing the errno magic when the close is deep\ninside a callstack, and passing out the two cases is awkward.\n\n-- >8 --\nSubject: tempfile: set errno to a known value before calling ferror()\n\nIn close_tempfile(), we return an error if ferror()\nindicated a previous failure, or if fclose() failed. In the\nlatter case, errno is set and it is useful for callers to\nreport it.\n\nHowever, if _only_ ferror() triggers, then the value of\nerrno is based on whatever syscall happened to last fail,\nwhich may not be related to our filehandle at all. A caller\ncannot tell the difference between the two cases, and may\nuse \"die_errno()\" or similar to report a nonsense errno value.\n\nOne solution would be to actually pass back separate return\nvalues for the two cases, so a caller can write a more\nappropriate message for each case. But that makes the\ninterface clunky.\n\nInstead, let's just set errno to the generic EIO in this case.\nThat's not as descriptive as we'd like, but at least it's\npredictable. So it's better than the status quo in all cases\nbut one: when the last syscall really did involve a failure\non our filehandle, we'll be wiping that out. But that's a\nfragile thing for us to rely on.\n\nIn any case, we'll let the errno result from fclose() take\nprecedence over our value, as we know that's recent and\naccurate (and many I/O errors will persist through the\nfclose anyway).\n\nSigned-off-by: Jeff King \n---\n tempfile.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex ffcc27237..684371067 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -247,8 +247,13 @@ int close_tempfile(struct tempfile *tempfile)\n \ttempfile->fd = -1;\n \tif (fp) {\n \t\ttempfile->fp = NULL;\n-\t\terr = ferror(fp);\n-\t\terr |= fclose(fp);\n+\t\tif (ferror(fp)) {\n+\t\t\terr = -1;\n+\t\t\tif (!fclose(fp))\n+\t\t\t\terrno = EIO;\n+\t\t} else {\n+\t\t\terr = fclose(fp);\n+\t\t}\n \t} else {\n \t\terr = close(fd);\n \t}\n-- \n2.12.0.rc1.612.ga5f664feb\n\n"},{"id":"311937","messageId":"xmqq37fcsejx.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T21:17:06Z","receivedAt":"2017-02-17T21:17:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:\n>\n>> On 02/17/2017 09:07 AM, Jeff King wrote:\n>> > [...]\n>> > That's similar to what I wrote earlier, but if we don't mind overwriting\n>> > errno unconditionally, I think just:\n>> > \n>> > errno = EIO; /* covers ferror(), overwritten by failing fclose() */\n>> > err |= ferror(fp);\n>> > err |= fclose(fp);\n>> > \n>> > does the same thing.\n>> \n>> True; I'd forgotten the convention that non-failing functions are\n>> allowed to change errno. Your solution is obviously simpler and faster.\n>\n> I guess we are simultaneously assuming that it is OK to munge errno on\n> success in our function, but that fclose() will not do so. Which seems a\n> bit hypocritical. Maybe the \"if\" dance is better.\n\nYes. When both ferror() and fclose() are successful, we would\nprefer to see the original errno unmolested, so the \"if\" dance,\neven though it looks uglier, is better. The ugliness is limited\nto the implementation anyway ;-)\n\nBut it does look ugly, especially when nested inside the existing\ncode like so.\n\nStepping back a bit, would this be really needed? Even if the ferror()\ndoes not update errno, the original stdio operation that failed\nwould have, no?\n\n-- >8 --\nSubject: close_tempfile(): set errno when ferror() notices a previous error\n\nIn close_tempfile(), we may notice that previous stdio operations\nfailed when we inspect ferror(tempfile->fp). As ferror() does not\nset errno, and the caller of close_tempfile(), since it encountered\nand ignored the original error, is likely to have called other\nsystem library functions to cause errno to be modified, the caller\ncannot really tell anything meaningful by looking at errno after we\nreturn an error from here. \n\nSet errno to an arbitrary value EIO when ferror() sees an error but\nfclose() succeeds. If fclose() fails, we just let the caller see\nerrno from that failure.\n\n---\n tempfile.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/tempfile.c b/tempfile.c\nindex ffcc272375..d2c6de83a9 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -247,8 +247,20 @@ int close_tempfile(struct tempfile *tempfile)\n \ttempfile->fd = -1;\n \tif (fp) {\n \t\ttempfile->fp = NULL;\n-\t\terr = ferror(fp);\n-\t\terr |= fclose(fp);\n+\t\tif (ferror(fp)) {\n+\t\t\terr = -1;\n+\t\t\tif (!fclose(fp))\n+\t\t\t\t/*\n+\t\t\t\t * There was some error detected by ferror()\n+\t\t\t\t * but it is likely that the true errno has\n+\t\t\t\t * long gone. Leave something generic to make\n+\t\t\t\t * it clear that the caller cannot rely on errno\n+\t\t\t\t * at this point.\n+\t\t\t\t */\n+\t\t\t\terrno = EIO;\n+\t\t} else {\n+\t\t\terr = fclose(fp);\n+\t\t}\n \t} else {\n \t\terr = close(fd);\n \t}\n"},{"id":"311938","messageId":"20170217212106.bew6krtb7pqpi3rr@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqq37fcsejx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T21:21:06Z","receivedAt":"2017-02-17T21:22:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:\n\n> > I guess we are simultaneously assuming that it is OK to munge errno on\n> > success in our function, but that fclose() will not do so. Which seems a\n> > bit hypocritical. Maybe the \"if\" dance is better.\n> \n> Yes. When both ferror() and fclose() are successful, we would\n> prefer to see the original errno unmolested, so the \"if\" dance,\n> even though it looks uglier, is better. The ugliness is limited\n> to the implementation anyway ;-)\n> \n> But it does look ugly, especially when nested inside the existing\n> code like so.\n\nI think our emails crossed, but our patches are obviously quite similar.\nMy commit message maybe explains a bit more of my thinking.\n\n> Stepping back a bit, would this be really needed? Even if the ferror()\n> does not update errno, the original stdio operation that failed\n> would have, no?\n\nSure, but we have no clue what happened in between.\n\n-Peff\n"},{"id":"311940","messageId":"xmqqy3x4qyte.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170217212106.bew6krtb7pqpi3rr@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T21:42:21Z","receivedAt":"2017-02-17T21:42:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:\n>\n>> Stepping back a bit, would this be really needed? Even if the ferror()\n>> does not update errno, the original stdio operation that failed\n>> would have, no?\n>\n> Sure, but we have no clue what happened in between.\n\nHmm, so we are protecting against somebody who does \"errno = 0\"\nexplicitly, because she knows that she's dealt with the error from\nstdio earlier? Such a careful person would have called clearerr()\nas well, I would guess.\n\nSo let's assume we only care about the case where some other error\nwas detected and errno was updated by a system library call.\n\n> I think our emails crossed, but our patches are obviously quite similar.\n> My commit message maybe explains a bit more of my thinking.\n\nYes, but ;-)\n\nIf we are trying to make sure that the caller would not say \"failed\nto close tempfile: ERRNO\" with an ERRNO that is unrelated to any\nstdio opration, I am not sure if the patch improves things. The\ncaller did not fail to close (most likely we successfully closed\nit), and no matter what futzing we do to errno, the message supplied\nby such a caller will not be improved.\n\nIf the caller used \"noticed an earlier error while closing tempfile:\nERRNO\", such a message would describe the situation more correctly,\nbut then ERRNO that is not about stdio is probably acceptable in the\ncontext of that message (the original ERRNO might be ENOSPC that is\neven more specific than EIO, FWIW). So I am not sure if the things\nwill improve from the status quo.\n\nIt's easy for us to either apply the patch and be done with it (or\ndrop and do the same), and in the bigger picture it wouldn't make\nthat much of a difference, I would think, so I can go either way.\n\n\n"},{"id":"311947","messageId":"20170217221019.wjuaxmaatqtx2olt@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqqy3x4qyte.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T22:10:19Z","receivedAt":"2017-02-17T22:17:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:\n> >\n> >> Stepping back a bit, would this be really needed? Even if the ferror()\n> >> does not update errno, the original stdio operation that failed\n> >> would have, no?\n> >\n> > Sure, but we have no clue what happened in between.\n> \n> Hmm, so we are protecting against somebody who does \"errno = 0\"\n> explicitly, because she knows that she's dealt with the error from\n> stdio earlier? Such a careful person would have called clearerr()\n> as well, I would guess.\n\nI'm not sure I understand what you are saying here. If somebody calls\nclearerr(), our ferror() handling does not trigger at all, and do not\ncare what is in errno either way. They can reset errno or not when they\nclearerr(), but it is not relevant.\n\nIf you are asking about somebody who sets errno to \"0\" and _doesn't_\ncall clearerr(), then I don't know what that person is trying to\naccomplish. Setting errno to \"0\" is not the right way to clear an error.\nAnd they certainly should not be relying on it not to get overwritten\nbefore we make it to the final ferror()/fclose().\n\n> So let's assume we only care about the case where some other error\n> was detected and errno was updated by a system library call.\n\nRight.\n\n> > I think our emails crossed, but our patches are obviously quite similar.\n> > My commit message maybe explains a bit more of my thinking.\n> \n> Yes, but ;-)\n> \n> If we are trying to make sure that the caller would not say \"failed\n> to close tempfile: ERRNO\" with an ERRNO that is unrelated to any\n> stdio opration, I am not sure if the patch improves things. The\n> caller did not fail to close (most likely we successfully closed\n> it), and no matter what futzing we do to errno, the message supplied\n> by such a caller will not be improved.\n\nRight. EIO is almost certainly _not_ the error we saw. But I would\nrather consistently say \"I/O error\" and have the user scratch their\nhead, look up this thread, and say \"ah, it was probably a deferred\nerror\", as opposed to the alternative: the user sees something\nnonsensical like ENOMEM or EBADF. Those are more misleading, and worse,\nmay change from run to run based on what other code runs or fails in\nbetween.\n\n> If the caller used \"noticed an earlier error while closing tempfile:\n> ERRNO\", such a message would describe the situation more correctly,\n> but then ERRNO that is not about stdio is probably acceptable in the\n> context of that message (the original ERRNO might be ENOSPC that is\n> even more specific than EIO, FWIW). So I am not sure if the things\n> will improve from the status quo.\n\nYes, that's I suggested that xfclose() is probably not a good direction.\nThe _best_ thing we can do is have the caller not report errno at all\n(or even say \"there was an earlier error, I have no idea what errno\nwas\"). And xfclose() works in the opposite direction.\n\nThe only reason I do not think we should do so for close_tempfile() is\nthat the fclose is typically far away from the code that actually calls\nerror(). We'd have to pass the tristate (success, fail, fail-with-errno)\nstate up through the stack (most of the calls indirectly come from\ncommit_lock_file(), I would think).\n\n-Peff\n"},{"id":"311957","messageId":"xmqqzihkphkc.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170217221019.wjuaxmaatqtx2olt@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T22:40:19Z","receivedAt":"2017-02-17T22:40:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> If we are trying to make sure that the caller would not say \"failed\n>> to close tempfile: ERRNO\" with an ERRNO that is unrelated to any\n>> stdio opration, I am not sure if the patch improves things. The\n>> caller did not fail to close (most likely we successfully closed\n>> it), and no matter what futzing we do to errno, the message supplied\n>> by such a caller will not be improved.\n>\n> Right. EIO is almost certainly _not_ the error we saw. But I would\n> rather consistently say \"I/O error\" and have the user scratch their\n> head, look up this thread, and say \"ah, it was probably a deferred\n> error\", as opposed to the alternative: the user sees something\n> nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,\n> may change from run to run based on what other code runs or fails in\n> between.\n\nMy point was actually not what errno we feed to strerror(). In that\nexample, what is more misleading is the fixed part of the error\nmessage the caller of close_tempfile() used after seeing the funcion\nfail, i.e. \"failed to close\". strerror() part is used to explain\nwhy we \"failed to close\", and of course any nonsensical errno that\nwe did not get from the failed stdio call would not explain it, but\na more misleading part is that we did not even \"failed to close\" it.\n\nWe just noticed an earlier error while attempting to close it.\nstrerror() in the message does not even have to be related to the\nclosing of the file handle.\n\n>> If the caller used \"noticed an earlier error while closing tempfile:\n>> ERRNO\", such a message would describe the situation more correctly,\n>> but then ERRNO that is not about stdio is probably acceptable in the\n>> context of that message (the original ERRNO might be ENOSPC that is\n>> even more specific than EIO, FWIW). So I am not sure if the things\n>> will improve from the status quo.\n>\n> Yes, that's I suggested that xfclose() is probably not a good direction.\n> The _best_ thing we can do is have the caller not report errno at all\n> (or even say \"there was an earlier error, I have no idea what errno\n> was\"). And xfclose() works in the opposite direction.\n\nI think we are in agreement on this point ;-)\n\n> The only reason I do not think we should do so for close_tempfile() is\n> that the fclose is typically far away from the code that actually calls\n> error(). We'd have to pass the tristate (success, fail, fail-with-errno)\n> state up through the stack (most of the calls indirectly come from\n> commit_lock_file(), I would think).\n\nWe _could_ clear errno to allow caller to tell them apart, though,\nif we wanted to ;-)\n"},{"id":"311984","messageId":"20170217233952.56h2z3l76orn4zht@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqqzihkphkc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T23:39:53Z","receivedAt":"2017-02-17T23:39:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:\n\n> > Right. EIO is almost certainly _not_ the error we saw. But I would\n> > rather consistently say \"I/O error\" and have the user scratch their\n> > head, look up this thread, and say \"ah, it was probably a deferred\n> > error\", as opposed to the alternative: the user sees something\n> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,\n> > may change from run to run based on what other code runs or fails in\n> > between.\n> \n> My point was actually not what errno we feed to strerror(). In that\n> example, what is more misleading is the fixed part of the error\n> message the caller of close_tempfile() used after seeing the funcion\n> fail, i.e. \"failed to close\". strerror() part is used to explain\n> why we \"failed to close\", and of course any nonsensical errno that\n> we did not get from the failed stdio call would not explain it, but\n> a more misleading part is that we did not even \"failed to close\" it.\n> \n> We just noticed an earlier error while attempting to close it.\n> strerror() in the message does not even have to be related to the\n> closing of the file handle.\n\nAh, I see. I think the errno thing is a strict improvement over what is\nthere now. Actually having a separate error message is even better, but\nit does end up rather verbose in the callers.\n\nI'm also not sure that it's all that useful to distinguish errors from\nfwrite() versus fclose(). In practice, errors will come from write() in\neither case, and the caller does not have much control over when the\nflushing happens. So any error saying \"error closing file\" is probably\nassuming too much anyway. It should be \"error writing file\".\n\nAnd I think in practice the messages end up quite generic anyway, as\nthey are really calling commit_lock_file(), which may also fail due to a\nrename. So you get something like \"unable to write 'foo': \", with errno\nappended. That's _also_ potentially confusing when rename() fails.\n\nSolving that would probably require passing down an \"err\" strbuf (or\nother data structure) for the low-level code to fill in.\n\n> > The only reason I do not think we should do so for close_tempfile() is\n> > that the fclose is typically far away from the code that actually calls\n> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)\n> > state up through the stack (most of the calls indirectly come from\n> > commit_lock_file(), I would think).\n> \n> We _could_ clear errno to allow caller to tell them apart, though,\n> if we wanted to ;-)\n\nHmm. So basically \"errno = 0\" instead of EIO? That at least has the\nadvantage that you can tell it apart from a real EIO, and a caller\n_could_ if they chose do:\n\n if (commit_lock_file(&lock)) {\n\tif (!errno)\n\t\terror(\"error writing to file\");\n\telse\n\t\terror_errno(\"error closing file\");\n }\n\nBut I am not sure I would want to annotate all such callers that way. It\nwould probably be less bad to just pass down a \"quiet\" flag or a strbuf\nand have the low-level code fill it in. And that solves this problem\n_and_ the rename() thing above.\n\nBut TBH, I am not sure if it is worth it. Nobody is complaining. This is\njust a thing we noticed. I think setting errno to EIO or to \"0\" is a\nstrict improvement over what is there (where the errno is essentially\nrandom) and the complexity doesn't escape the function.\n\nSo I think that \"easy\" thing falls far short of a solution, but it's at\nleast easy. I could take it or leave it at this point.\n\n-Peff\n"},{"id":"311994","messageId":"20170217235413.zuwqabfo7jtw3lz7@sigill.intra.peff.net","threadId":"45120","inReplyTo":"xmqqr32wnznf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-17T23:54:13Z","receivedAt":"2017-02-17T23:54:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 17, 2017 at 03:52:36PM -0800, Junio C Hamano wrote:\n\n> > So I think that \"easy\" thing falls far short of a solution, but it's at\n> > least easy. I could take it or leave it at this point.\n> \n> Well, I already said that earlier in this thread, and ended up\n> queuing your patch on 'pu' ;-).\n\nWe're slowly converging on consensus in our apathy. ;)\n\n-Peff\n"},{"id":"311996","messageId":"xmqqr32wnznf.fsf@gitster.mtv.corp.google.com","threadId":"45120","inReplyTo":"20170217233952.56h2z3l76orn4zht@sigill.intra.peff.net","subject":"Re: [PATCH] tempfile: avoid \"ferror | fclose\" trick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-17T23:52:36Z","receivedAt":"2017-02-18T00:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm also not sure that it's all that useful to distinguish errors from\n> fwrite() versus fclose(). In practice, errors will come from write() in\n> either case, and the caller does not have much control over when the\n> flushing happens. So any error saying \"error closing file\" is probably\n> assuming too much anyway. It should be \"error writing file\".\n\nYes.\n\n>> We _could_ clear errno to allow caller to tell them apart, though,\n>> if we wanted to ;-)\n>\n> Hmm. So basically \"errno = 0\" instead of EIO? That at least has the\n> advantage that you can tell it apart from a real EIO, and a caller\n> _could_ if they chose do:\n>\n> if (commit_lock_file(&lock)) {\n> \tif (!errno)\n> \t\terror(\"error writing to file\");\n> \telse\n> \t\terror_errno(\"error closing file\");\n> }\n\nExactly.\n\n> But I am not sure I would want to annotate all such callers that way. It\n> would probably be less bad to just pass down a \"quiet\" flag or a strbuf\n> and have the low-level code fill it in. And that solves this problem\n> _and_ the rename() thing above.\n>\n> But TBH, I am not sure if it is worth it. Nobody is complaining. This is\n> just a thing we noticed. I think setting errno to EIO or to \"0\" is a\n> strict improvement over what is there (where the errno is essentially\n> random) and the complexity doesn't escape the function.\n>\n> So I think that \"easy\" thing falls far short of a solution, but it's at\n> least easy. I could take it or leave it at this point.\n\nWell, I already said that earlier in this thread, and ended up\nqueuing your patch on 'pu' ;-).\n"}]}