{"thread":{"id":"39813","subject":"[PATCH] git_open_noatime: return with errno=0 on success","startedAt":"2015-07-08T12:38:20Z","lastAt":"2015-08-05T16:27:12Z","messageCount":7,"participants":["Clemens Buchacher","Eric Sunshine","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265845","messageId":"20150708123820.GA25269@musxeris015.imu.intel.com","threadId":"39813","inReplyTo":null,"subject":"[PATCH] git_open_noatime: return with errno=0 on success","fromName":"Clemens Buchacher","fromEmail":"clemens.buchacher@intel.com","sentAt":"2015-07-08T12:38:20Z","receivedAt":"2015-07-08T12:38:20Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In read_sha1_file_extended we die if read_object fails with a fatal\nerror. We detect a fatal error if errno is non-zero and is not\nENOENT. If the object could not be read because it does not exist,\nthis is not considered a fatal error and we want to return NULL.\n\nSomewhere down the line, read_object calls git_open_noatime to open\na pack index file, for example. We first try open with O_NOATIME.\nIf O_NOATIME fails with EPERM, we retry without O_NOATIME. When the\nsecond open succeeds, errno is however still set to EPERM from the\nfirst attemt. When we finally determine that the object does not\nexist, read_object returns NULL and read_sha1_file_extended dies\nwith a fatal error:\n\n fatal: failed to read object : Operation not permitted\n\nFix this by resetting errno to zero before we call open again.\n\nCc: Linus Torvalds \nSigned-off-by: Clemens Buchacher \nHelped-by: Martin Schröder \n---\n sha1_file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 77cd81d..62b7ad6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1453,6 +1453,7 @@ int git_open_noatime(const char *name)\n \tstatic int sha1_file_open_flag = O_NOATIME;\n \n \tfor (;;) {\n+\t\terrno = 0;\n \t\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n-- \n1.9.4\n"},{"id":"265868","messageId":"CAPig+cSacM_JwZzagOVZpMJF=oE7m3rMnq1eKr=aNsGY0vvmfQ@mail.gmail.com","threadId":"39813","inReplyTo":"20150708123820.GA25269@musxeris015.imu.intel.com","subject":"Re: [PATCH] git_open_noatime: return with errno=0 on success","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-08T18:51:31Z","receivedAt":"2015-07-08T18:51:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 8, 2015 at 8:38 AM, Clemens Buchacher\n wrote:\n> In read_sha1_file_extended we die if read_object fails with a fatal\n> error. We detect a fatal error if errno is non-zero and is not\n> ENOENT. If the object could not be read because it does not exist,\n> this is not considered a fatal error and we want to return NULL.\n>\n> Somewhere down the line, read_object calls git_open_noatime to open\n> a pack index file, for example. We first try open with O_NOATIME.\n> If O_NOATIME fails with EPERM, we retry without O_NOATIME. When the\n> second open succeeds, errno is however still set to EPERM from the\n> first attemt. When we finally determine that the object does not\n\ns/attemt/attempt/\n\n> exist, read_object returns NULL and read_sha1_file_extended dies\n> with a fatal error:\n>\n> fatal: failed to read object : Operation not permitted\n>\n> Fix this by resetting errno to zero before we call open again.\n>\n> Cc: Linus Torvalds \n> Signed-off-by: Clemens Buchacher \n> Helped-by: Martin Schröder \n"},{"id":"267386","messageId":"20150804082429.GA22271@musxeris015.imu.intel.com","threadId":"39813","inReplyTo":"CAPig+cSacM_JwZzagOVZpMJF=oE7m3rMnq1eKr=aNsGY0vvmfQ@mail.gmail.com","subject":"[PATCH] git_open_noatime: return with errno=0 on success","fromName":"Clemens Buchacher","fromEmail":"clemens.buchacher@intel.com","sentAt":"2015-08-04T08:24:29Z","receivedAt":"2015-08-04T08:24:29Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In read_sha1_file_extended we die if read_object fails with a fatal\nerror. We detect a fatal error if errno is non-zero and is not\nENOENT. If the object could not be read because it does not exist,\nthis is not considered a fatal error and we want to return NULL.\n\nSomewhere down the line, read_object calls git_open_noatime to open\na pack index file, for example. We first try open with O_NOATIME.\nIf O_NOATIME fails with EPERM, we retry without O_NOATIME. When the\nsecond open succeeds, errno is however still set to EPERM from the\nfirst attempt. When we finally determine that the object does not\nexist, read_object returns NULL and read_sha1_file_extended dies\nwith a fatal error:\n\n fatal: failed to read object : Operation not permitted\n\nFix this by resetting errno to zero before we call open again.\n\nCc: Linus Torvalds \nSigned-off-by: Clemens Buchacher \n---\n\nThis is a re-submission without changes except for a typo fix in the\ncomments (thanks Eric). The original submission received no other\ncomments, but I think it is a clear improvement and I hope it was just\nmissed the first time.\n\nBest regards,\nClemens\n\n sha1_file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 77cd81d..62b7ad6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1453,6 +1453,7 @@ int git_open_noatime(const char *name)\n \tstatic int sha1_file_open_flag = O_NOATIME;\n \n \tfor (;;) {\n+\t\terrno = 0;\n \t\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n-- \n1.9.4\n"},{"id":"267478","messageId":"xmqqfv3y6a24.fsf@gitster.dls.corp.google.com","threadId":"39813","inReplyTo":"20150804082429.GA22271@musxeris015.imu.intel.com","subject":"Re: [PATCH] git_open_noatime: return with errno=0 on success","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-04T21:03:15Z","receivedAt":"2015-08-04T21:03:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 77cd81d..62b7ad6 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1453,6 +1453,7 @@ int git_open_noatime(const char *name)\n> \tstatic int sha1_file_open_flag = O_NOATIME;\n> \n> \tfor (;;) {\n> +\t\terrno = 0;\n> \t\tint fd = open(name, O_RDONLY | sha1_file_open_flag);\n\nPlease avoid decl-after-stmt, which this codebase does not accept.\n\n> \t\tif (fd >= 0)\n> \t\t\treturn fd;\n\nMore importantly, is this the right place to clear errno?\n\nI would agree it is a good idea to clear it after seeing the first\nopen fail due to lack of O_NOATIME before trying open for the second\ntime, iow, more like this?\n\n\n sha1_file.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1cee438..bf2f229 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1467,6 +1467,7 @@ int git_open_noatime(const char *name)\n \n \t\t/* Might the failure be due to O_NOATIME? */\n \t\tif (errno != ENOENT && sha1_file_open_flag) {\n+\t\t\terrno = 0;\n \t\t\tsha1_file_open_flag = 0;\n \t\t\tcontinue;\n \t\t}\n"},{"id":"267502","messageId":"CA+55aFx-WXxCbVTWdJHFf4WA2MNXS3UMerv4cD1wtsZGaQkJLw@mail.gmail.com","threadId":"39813","inReplyTo":"xmqqfv3y6a24.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git_open_noatime: return with errno=0 on success","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-08-05T08:59:09Z","receivedAt":"2015-08-05T08:59:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Tue, Aug 4, 2015 at 11:03 PM, Junio C Hamano wrote:\n>\n> I would agree it is a good idea to clear it after seeing the first\n> open fail due to lack of O_NOATIME before trying open for the second\n> time, iow, more like this?\n\nSo I don't think this is _wrong_ per se, but I think the deeper issue\nis that somebody cares about 'errno' here in the first place.\n\nA stale 'errno' generally shouldn't matter, because we either\n\n (a) return success (and nobody should look at errno)\n\nor\n\n (b) return an error later, without setting errno for that _later_ error.\n\nand I think either of those two situations are the real bug, and this\n\"clear stale errno\" is just a workaround.\n\nBut as mentioned, I don't think clearign errno is wrong, so I'm not\nobjecting to the patch. I just suspect there's something else goign on\ntoo..\n\n Linus\n"},{"id":"267514","messageId":"20150805143600.GA3111@musxeris015.imu.intel.com","threadId":"39813","inReplyTo":"CA+55aFx-WXxCbVTWdJHFf4WA2MNXS3UMerv4cD1wtsZGaQkJLw@mail.gmail.com","subject":"Re: [PATCH] git_open_noatime: return with errno=0 on success","fromName":"Clemens Buchacher","fromEmail":"clemens.buchacher@intel.com","sentAt":"2015-08-05T14:36:00Z","receivedAt":"2015-08-05T14:36:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Aug 05, 2015 at 10:59:09AM +0200, Linus Torvalds wrote:\n> On Tue, Aug 4, 2015 at 11:03 PM, Junio C Hamano wrote:\n> >\n> > I would agree it is a good idea to clear it after seeing the first\n> > open fail due to lack of O_NOATIME before trying open for the second\n> > time, iow, more like this?\n\nLooks good to me.\n\n> So I don't think this is _wrong_ per se, but I think the deeper issue\n> is that somebody cares about 'errno' here in the first place.\n> \n> A stale 'errno' generally shouldn't matter, because we either\n> \n> (a) return success (and nobody should look at errno)\n> \n> or\n> \n> (b) return an error later, without setting errno for that _later_ error.\n> \n> and I think either of those two situations are the real bug, and this\n> \"clear stale errno\" is just a workaround.\n\nI agree. But I do not see how to get there easily.\n\nWe are trying to read an object. We first try to read from a pack. We\nmay encounter broken pack files, missing index files, unreadable files,\nbut those errors are not necessarily fatal since we may still be able to\nread the object from the next pack file or from a sha1 file.\n\nIf finally we do not find the object anywhere, in\nread_sha1_file_extended we try our best to die with an appropriate error\nmessage, for example by looking at errno, and otherwise we just return\nNULL. Most callers seem to die explicitly or they dereference the null\npointer.\n\nI think we should instead output error messages closer to the source,\nlike for example in map_sha1_file, but continue anyway. In particular we\nshould immediately report failures due to EPERM or unexpected ENOENT. In\nthe end we may return NULL without another message, but at least the\nuser should have some hints about what went wrong along the way.\n"},{"id":"267517","messageId":"xmqqvbct4s67.fsf@gitster.dls.corp.google.com","threadId":"39813","inReplyTo":"20150805143600.GA3111@musxeris015.imu.intel.com","subject":"Re: [PATCH] git_open_noatime: return with errno=0 on success","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T16:27:12Z","receivedAt":"2015-08-05T16:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Aug 05, 2015 at 10:59:09AM +0200, Linus Torvalds wrote:\n> ...\n>> A stale 'errno' generally shouldn't matter, because we either\n>> \n>> (a) return success (and nobody should look at errno)\n>> \n>> or\n>> \n>> (b) return an error later, without setting errno for that _later_ error.\n>> \n>> and I think either of those two situations are the real bug, and this\n>> \"clear stale errno\" is just a workaround.\n>\n> I agree. But I do not see how to get there easily.\n>\n> We are trying to read an object. We first try to read from a pack. We\n> may encounter broken pack files, missing index files, unreadable files,\n> but those errors are not necessarily fatal since we may still be able to\n> read the object from the next pack file or from a sha1 file.\n>\n> If finally we do not find the object anywhere, in\n> read_sha1_file_extended we try our best to die with an appropriate error\n> message, for example by looking at errno, and otherwise we just return\n> NULL. Most callers seem to die explicitly or they dereference the null\n> pointer.\n>\n> I think we should instead output error messages closer to the source,\n> like for example in map_sha1_file, but continue anyway.\n\nHmm, if we find one data source unreadable but an alternative\nusable, do we really want that error message? What should it say?\n\"error: cannot read from pack\"? Such a message, unless we later\ngive \"info: but we managed to read it from elsewhere\" and make sure\nthese two messages are clearly associated with each other, would\nmake things unnecessarily alarming, wouldn't it?\n\nPerhaps we should not rely so heavily on 'errno', but explicitly\npass around error code (or enough information to formulate an\nintelligent message at the end) in the callchain instead.\n\nThen the earlier part can notice EPERM on a pack, for example, and\nreturn to the caller, and after consulting an alternate data source\n(e.g. loose object file), the caller can then choose to say \"we\nmanaged to read the data, but FYI, you may want to check the\npermission bits of this pack\", or choose to stay silent.\n"}]}