{"thread":{"id":"48187","subject":"[PATCH] config: move flockfile() closer to unlocked functions","startedAt":"2018-03-30T19:26:22Z","lastAt":"2018-03-30T19:52:17Z","messageCount":2,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343467","messageId":"20180330192615.GB32338@sigill.intra.peff.net","threadId":"48187","inReplyTo":null,"subject":"[PATCH] config: move flockfile() closer to unlocked functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-30T19:26:15Z","receivedAt":"2018-03-30T19:26:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 30, 2018 at 09:04:13PM +0200, Johannes Schindelin wrote:\n\n> > Probably the flockfile should go into do_config_from_file(), where we\n> > specify to use the unlocked variants.\n> \n> Ah, that makes sense now! I am glad I could also help ;-)\n\n:)\n\n> > Yeah, I'll wait to see how your refactor turns out.\n> \n> I don't think I'll touch too much in that part of the code. My changes\n> should not cause merge conflicts with a patch moving the\n> flockfile()/funlockfile() calls to do_config_from_file().\n\nOK, then let's do this while we're thinking about it:\n\n-- >8 --\nSubject: config: move flockfile() closer to unlocked functions\n\nCommit 260d408e32 (config: use getc_unlocked when reading\nfrom file, 2015-04-16) taught git_config_from_file() to lock\nthe filehandle so that we could safely use the faster\nunlocked functions to access the handle.\n\nHowever, it split the logic into two places:\n\n 1. The master lock/unlock happens in git_config_from_file().\n\n 2. The decision to use the unlocked functions happens in\n do_config_from_file().\n\nThat means that if anybody calls the latter function, they\nwill accidentally use the unlocked functions without holding\nthe lock. And indeed, git_config_from_stdin() does so.\n\nIn practice, this hasn't been a problem since this code\nisn't generally multi-threaded (and even if some Git program\nhappened to have another thread running, it's unlikely to be\nreading from stdin). But it's a good practice to make sure\nwe're always holding the lock before using the unlocked\nfunctions.\n\nHelped-by: Johannes Schindelin \nSigned-off-by: Jeff King \n---\nI wasn't sure if this was \"helped by\" or \"reported by\" or\n\"stumbled-upon-by\". :)\n\n config.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb8..609ef2f58b 100644\n--- a/config.c\n+++ b/config.c\n@@ -1426,6 +1426,7 @@ static int do_config_from_file(config_fn_t fn,\n \t\tvoid *data)\n {\n \tstruct config_source top;\n+\tint ret;\n \n \ttop.u.file = f;\n \ttop.origin_type = origin_type;\n@@ -1436,7 +1437,10 @@ static int do_config_from_file(config_fn_t fn,\n \ttop.do_ungetc = config_file_ungetc;\n \ttop.do_ftell = config_file_ftell;\n \n-\treturn do_config_from(&top, fn, data);\n+\tflockfile(f);\n+\tret = do_config_from(&top, fn, data);\n+\tfunlockfile(f);\n+\treturn ret;\n }\n \n static int git_config_from_stdin(config_fn_t fn, void *data)\n@@ -1451,9 +1455,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n \tf = fopen_or_warn(filename, \"r\");\n \tif (f) {\n-\t\tflockfile(f);\n \t\tret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);\n-\t\tfunlockfile(f);\n \t\tfclose(f);\n \t}\n \treturn ret;\n-- \n2.17.0.rc2.594.gdb94a0ce02\n\n"},{"id":"343472","messageId":"xmqqzi2p1gus.fsf@gitster-ct.c.googlers.com","threadId":"48187","inReplyTo":"20180330192615.GB32338@sigill.intra.peff.net","subject":"Re: [PATCH] config: move flockfile() closer to unlocked functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T19:52:11Z","receivedAt":"2018-03-30T19:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 30, 2018 at 09:04:13PM +0200, Johannes Schindelin wrote:\n>\n>> > Probably the flockfile should go into do_config_from_file(), where we\n>> > specify to use the unlocked variants.\n>> \n>> Ah, that makes sense now! I am glad I could also help ;-)\n>\n> :)\n>\n>> > Yeah, I'll wait to see how your refactor turns out.\n>> \n>> I don't think I'll touch too much in that part of the code. My changes\n>> should not cause merge conflicts with a patch moving the\n>> flockfile()/funlockfile() calls to do_config_from_file().\n>\n> OK, then let's do this while we're thinking about it:\n\nYup, what Dscho found was quite amusing ;-) and this obviously makes\nthe code clearer to follow.\n\nWill queue, thanks.\n\n"}]}