{"thread":{"id":"48420","subject":"[PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","startedAt":"2018-05-06T14:11:12Z","lastAt":"2018-05-08T18:08:47Z","messageCount":6,"participants":["Martin Ågren","David Turner","Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"346764","messageId":"20180506141031.30204-3-martin.agren@gmail.com","threadId":"48420","inReplyTo":null,"subject":"[PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T14:10:28Z","receivedAt":"2018-05-06T14:11:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"If we could not take the lock, we add an error to the `strbuf err` and\nreturn. However, this code is dead. The reason is that we take the lock\nusing `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle\nerror-handling to actually kick in.\n\nWe could instead just drop the dead code and die here. But everything is\nprepared for gently propagating the error, so let's do that instead.\n\nThere is similar dead code in `delete_pseudoref()`, but let's save that\nfor the next patch.\n\nWhile at it, make the lock non-static.\n\nSigned-off-by: Martin Ågren \n---\n refs.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8b7a77fe5e..8c50b8b139 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n {\n \tconst char *filename;\n \tint fd;\n-\tstatic struct lock_file lock;\n+\tstruct lock_file lock = LOCK_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret = -1;\n \n@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \tstrbuf_addf(&buf, \"%s\\n\", oid_to_hex(oid));\n \n \tfilename = git_path(\"%s\", pseudoref);\n-\tfd = hold_lock_file_for_update_timeout(&lock, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR,\n+\tfd = hold_lock_file_for_update_timeout(&lock, filename, 0,\n \t\t\t\t\t get_files_ref_lock_timeout_ms());\n \tif (fd < 0) {\n \t\tstrbuf_addf(err, \"could not open '%s' for writing: %s\",\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"346771","messageId":"1525621731.16035.11.camel@novalis.org","threadId":"48420","inReplyTo":"20180506141031.30204-3-martin.agren@gmail.com","subject":"Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-05-06T15:48:51Z","receivedAt":"2018-05-06T15:48:55Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Re making the lock static, I wonder about the following case:\n \n if (read_ref(pseudoref, &actual_old_oid))\n \ndie(\"could not read ref '%s'\", pseudoref);\n\nI think this calls exit(), and then atexit tries to clean up the lock\nfiles. But since lock is no longer static, the stack may have been\ndestroyed (I don't actually know whether this is true, so maybe someone\nelse does).\n\nOn Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:\n> If we could not take the lock, we add an error to the `strbuf err`\n> and\n> return. However, this code is dead. The reason is that we take the\n> lock\n> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle\n> error-handling to actually kick in.\n> \n> We could instead just drop the dead code and die here. But everything\n> is\n> prepared for gently propagating the error, so let's do that instead.\n> \n> There is similar dead code in `delete_pseudoref()`, but let's save\n> that\n> for the next patch.\n> \n> While at it, make the lock non-static.\n> \n> Signed-off-by: Martin Ågren \n> ---\n> refs.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 8b7a77fe5e..8c50b8b139 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref,\n> const struct object_id *oid,\n> {\n> \tconst char *filename;\n> \tint fd;\n> -\tstatic struct lock_file lock;\n> +\tstruct lock_file lock = LOCK_INIT;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tint ret = -1;\n> \n> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref,\n> const struct object_id *oid,\n> \tstrbuf_addf(&buf, \"%s\\n\", oid_to_hex(oid));\n> \n> \tfilename = git_path(\"%s\", pseudoref);\n> -\tfd = hold_lock_file_for_update_timeout(&lock, filename,\n> -\t\t\t\t\t LOCK_DIE_ON_ERROR,\n> +\tfd = hold_lock_file_for_update_timeout(&lock, filename, 0,\n> \t\t\t\t\t get_files_ref_lock_ti\n> meout_ms());\n> \tif (fd < 0) {\n> \t\tstrbuf_addf(err, \"could not open '%s' for writing:\n> %s\",\n"},{"id":"346773","messageId":"CAN0heSpfu+070fxhjjccXQpfFnr13O4tvSaj44YWkFWVJL_4mA@mail.gmail.com","threadId":"48420","inReplyTo":"1525621731.16035.11.camel@novalis.org","subject":"Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T15:56:31Z","receivedAt":"2018-05-06T15:56:35Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 6 May 2018 at 17:48, David Turner wrote:\n> On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:\n>> While at it, make the lock non-static.\n\n> Re making the lock static, I wonder about the following case:\n>\n> if (read_ref(pseudoref, &actual_old_oid))\n>\n> die(\"could not read ref '%s'\", pseudoref);\n>\n> I think this calls exit(), and then atexit tries to clean up the lock\n> files. But since lock is no longer static, the stack may have been\n> destroyed (I don't actually know whether this is true, so maybe someone\n> else does).\n\nRight. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on\nheap, 2017-09-05) this is safe though. Quite a few locks have already\nbeen moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles\nnon-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify locking\nlogic, 2017-10-05). I could add a note to the commit message to make\nthis clear, like \"After 076aa2cbda, locks no longer need to be static.\"\n\nMartin\n"},{"id":"346901","messageId":"01AF002E-C9E2-48FB-97AB-D9D903866310@novalis.org","threadId":"48420","inReplyTo":"CAN0heSpfu+070fxhjjccXQpfFnr13O4tvSaj44YWkFWVJL_4mA@mail.gmail.com","subject":"Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-05-07T11:12:27Z","receivedAt":"2018-05-07T18:32:56Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"\n\nOn May 6, 2018 9:56:31 AM MDT, \"Martin Ågren\" wrote:\n>On 6 May 2018 at 17:48, David Turner wrote:\n>> On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:\n>>> While at it, make the lock non-static.\n>\n>> Re making the lock static, I wonder about the following case:\n>>\n>> if (read_ref(pseudoref, &actual_old_oid))\n>>\n>> die(\"could not read ref '%s'\", pseudoref);\n>>\n>> I think this calls exit(), and then atexit tries to clean up the lock\n>> files. But since lock is no longer static, the stack may have been\n>> destroyed (I don't actually know whether this is true, so maybe\n>someone\n>> else does).\n>\n>Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on\n>heap, 2017-09-05) this is safe though. Quite a few locks have already\n>been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles\n>non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify\n>locking\n>logic, 2017-10-05). I could add a note to the commit message to make\n>this clear, like \"After 076aa2cbda, locks no longer need to be static.\"\n\nI am going to reply now to keep the thread moving, but I am on my phone with bad connectivity (few cell towers in Bears Ears), so I can't really check the code. Feel free to disregard if I am still wrong.\n\nI saw that patch, but I thought the new logic required that cleanup funtions be called before the lock goes out of scope.\n\n-- \nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"346904","messageId":"CAGZ79kZ=7uv3-2mr97uS4Y=1AS5u=koYfH=fMSWfu4L8ajNoKA@mail.gmail.com","threadId":"48420","inReplyTo":"20180506141031.30204-3-martin.agren@gmail.com","subject":"Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-07T18:48:51Z","receivedAt":"2018-05-07T18:48:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+cc Michael, who did extensive work in the refs.c code.\n\nOn Sun, May 6, 2018 at 7:10 AM, Martin Ågren wrote:\n> If we could not take the lock, we add an error to the `strbuf err` and\n> return. However, this code is dead. The reason is that we take the lock\n> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle\n> error-handling to actually kick in.\n>\n> We could instead just drop the dead code and die here. But everything is\n> prepared for gently propagating the error, so let's do that instead.\n\nThis looks good to me.\n\n> There is similar dead code in `delete_pseudoref()`, but let's save that\n> for the next patch.\n>\n> While at it, make the lock non-static.\n\nWe seem to have a lot of static lockfiles in the code base. IIRC that\nwas due to some technicality of the lockfiles, as they would also\nbe cleaned up atexit() and for that it had to be static(?)\n\nMaybe mention why it was static and why we can drop the static\nnow? Given that you found these answers in a reply below, this is\nReviewed-by: Stefan Beller \n\nThanks,\nStefan\n\n>\n> Signed-off-by: Martin Ågren \n> ---\n> refs.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 8b7a77fe5e..8c50b8b139 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n> {\n> const char *filename;\n> int fd;\n> - static struct lock_file lock;\n> + struct lock_file lock = LOCK_INIT;\n> struct strbuf buf = STRBUF_INIT;\n> int ret = -1;\n>\n> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n> strbuf_addf(&buf, \"%s\\n\", oid_to_hex(oid));\n>\n> filename = git_path(\"%s\", pseudoref);\n> - fd = hold_lock_file_for_update_timeout(&lock, filename,\n> - LOCK_DIE_ON_ERROR,\n> + fd = hold_lock_file_for_update_timeout(&lock, filename, 0,\n> get_files_ref_lock_timeout_ms());\n> if (fd < 0) {\n> strbuf_addf(err, \"could not open '%s' for writing: %s\",\n> --\n> 2.17.0.411.g9fd64c8e46\n>\n"},{"id":"347000","messageId":"20180508180840.GA7210@sigill.intra.peff.net","threadId":"48420","inReplyTo":"01AF002E-C9E2-48FB-97AB-D9D903866310@novalis.org","subject":"Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-08T18:08:41Z","receivedAt":"2018-05-08T18:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 07, 2018 at 05:12:27AM -0600, David Turner wrote:\n\n> >Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on\n> >heap, 2017-09-05) this is safe though. Quite a few locks have already\n> >been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles\n> >non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify\n> >locking\n> >logic, 2017-10-05). I could add a note to the commit message to make\n> >this clear, like \"After 076aa2cbda, locks no longer need to be static.\"\n> \n> I am going to reply now to keep the thread moving, but I am on my\n> phone with bad connectivity (few cell towers in Bears Ears), so I\n> can't really check the code. Feel free to disregard if I am still\n> wrong.\n> \n> I saw that patch, but I thought the new logic required that cleanup\n> funtions be called before the lock goes out of scope.\n\nNo, it should be fine. After 422a21c6a0 (tempfile: remove deactivated\nlist entries, 2017-09-05) it became _possible_ to use a non-static\ntempfile. But it was dangerous, because if you failed to clean up, bad\nthings would happen. So right after that in 076aa2cbda we switched to\nusing the heap, which means the tempfile code takes full ownership, and\nthe local lockfile variable is just a pointer to that storage.\n\n-Peff\n"}]}