Re: [PATCH 8/9] git_config_set: use do_config_from_file() directly

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 30, 2018, 14:01 UTC
Message-ID: <nycvar.QRO.7.76.6.1803301558360.5026@qfpub.tvgsbejvaqbjf.bet>
In-Reply-To: <20180330131456.GB29568@sigill.intra.peff.net>

Hi Peff,

On Fri, 30 Mar 2018, Jeff King wrote:

Show 29 quoted lines

> 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