Hi Peff,
On Fri, 30 Mar 2018, Jeff King wrote:
> On Fri, Mar 30, 2018 at 03:02:00PM +0200, Johannes Schindelin wrote:
>
> > > I'm not sure I understand that last paragraph. What does flockfile() have
> > > to do with stdin/stdout?
> > >
> > > The point of those calls is that we're locking the FILE handle, so that
> > > it's safe for the lower-level config code to run getc_unlocked(), which
> > > is faster.
> > >
> > > So without those, we're calling getc_unlocked() without holding the
> > > lock. I think it probably works in practice because we know that we're
> > > single-threaded, but it seems a bit sketchy.
> >
> > Oops. I misunderstood the purpose of flockfile(), then. I thought it was
> > only about multiple users of stdin/stdout.
> >
> > Will have a look whether flockfile()/funlockfile() can be moved into
> > do_config_from_file() instead.
>
> In a sense stdin/stdout are much more susceptible to this because
> they're global variables, and any thread may touch them. For the config
> code, we open our own handle that we don't expose elsewhere. So probably
> it would be fine just to use the unlocked variants even without locking.
>
> But IMHO it's good practice to always flockfile() before using the
> unlocked variants. My reading of POSIX is that it's OK to use the
> unlocked variants without holding the lock (if you know there won't be
> contention), but if it's not hard to err on the side of safety, I'd
> prefer it.
You know what is *really* funny?
-- snip --
static int git_config_from_stdin(config_fn_t fn, void *data)
{
return do_config_from_file(fn, CONFIG_ORIGIN_STDIN, "", NULL, stdin, data, 0);
}
int git_config_from_file(config_fn_t fn, const char *filename, void *data)
{
int ret = -1;
FILE *f;
f = fopen_or_warn(filename, "r");
if (f) {
flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data, 0);
funlockfile(f);
fclose(f);
}
return ret;
}
-- snap --
So the _stdin variant *goes out of its way not to flockfile()*...
But I guess all this will become moot when I start handing down the config
options. It does mean that I have to change the signatures in header
files, oh well ;-)
But then I can drop this here patch and we can stop musing about
flockfile() ;-)
Ciao,
Dscho
