Hi Peff,

On Fri, 30 Mar 2018, Jeff King wrote:

On Fri, Mar 30, 2018 at 03:02:00PM +0200, Johannes Schindelin wrote:

quoted

quoted

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