* [PATCH] config: move flockfile() closer to unlocked functions
@ 2018-03-30 19:26 Jeff King
2018-03-30 19:52 ` Junio C Hamano
0 siblings, 1 reply; 2+ messages in thread
From: Jeff King @ 2018-03-30 19:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
On Fri, Mar 30, 2018 at 09:04:13PM +0200, Johannes Schindelin wrote:
> > Probably the flockfile should go into do_config_from_file(), where we
> > specify to use the unlocked variants.
>
> Ah, that makes sense now! I am glad I could also help ;-)
:)
> > Yeah, I'll wait to see how your refactor turns out.
>
> I don't think I'll touch too much in that part of the code. My changes
> should not cause merge conflicts with a patch moving the
> flockfile()/funlockfile() calls to do_config_from_file().
OK, then let's do this while we're thinking about it:
-- >8 --
Subject: config: move flockfile() closer to unlocked functions
Commit 260d408e32 (config: use getc_unlocked when reading
from file, 2015-04-16) taught git_config_from_file() to lock
the filehandle so that we could safely use the faster
unlocked functions to access the handle.
However, it split the logic into two places:
1. The master lock/unlock happens in git_config_from_file().
2. The decision to use the unlocked functions happens in
do_config_from_file().
That means that if anybody calls the latter function, they
will accidentally use the unlocked functions without holding
the lock. And indeed, git_config_from_stdin() does so.
In practice, this hasn't been a problem since this code
isn't generally multi-threaded (and even if some Git program
happened to have another thread running, it's unlikely to be
reading from stdin). But it's a good practice to make sure
we're always holding the lock before using the unlocked
functions.
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Jeff King <peff@peff.net>
---
I wasn't sure if this was "helped by" or "reported by" or
"stumbled-upon-by". :)
config.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/config.c b/config.c
index b0c20e6cb8..609ef2f58b 100644
--- a/config.c
+++ b/config.c
@@ -1426,6 +1426,7 @@ static int do_config_from_file(config_fn_t fn,
void *data)
{
struct config_source top;
+ int ret;
top.u.file = f;
top.origin_type = origin_type;
@@ -1436,7 +1437,10 @@ static int do_config_from_file(config_fn_t fn,
top.do_ungetc = config_file_ungetc;
top.do_ftell = config_file_ftell;
- return do_config_from(&top, fn, data);
+ flockfile(f);
+ ret = do_config_from(&top, fn, data);
+ funlockfile(f);
+ return ret;
}
static int git_config_from_stdin(config_fn_t fn, void *data)
@@ -1451,9 +1455,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
f = fopen_or_warn(filename, "r");
if (f) {
- flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
- funlockfile(f);
fclose(f);
}
return ret;
--
2.17.0.rc2.594.gdb94a0ce02
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] config: move flockfile() closer to unlocked functions
2018-03-30 19:26 [PATCH] config: move flockfile() closer to unlocked functions Jeff King
@ 2018-03-30 19:52 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2018-03-30 19:52 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
Jeff King <peff@peff.net> writes:
> On Fri, Mar 30, 2018 at 09:04:13PM +0200, Johannes Schindelin wrote:
>
>> > Probably the flockfile should go into do_config_from_file(), where we
>> > specify to use the unlocked variants.
>>
>> Ah, that makes sense now! I am glad I could also help ;-)
>
> :)
>
>> > Yeah, I'll wait to see how your refactor turns out.
>>
>> I don't think I'll touch too much in that part of the code. My changes
>> should not cause merge conflicts with a patch moving the
>> flockfile()/funlockfile() calls to do_config_from_file().
>
> OK, then let's do this while we're thinking about it:
Yup, what Dscho found was quite amusing ;-) and this obviously makes
the code clearer to follow.
Will queue, thanks.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-03-30 19:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-30 19:26 [PATCH] config: move flockfile() closer to unlocked functions Jeff King
2018-03-30 19:52 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).