Re: [PATCH v2 18/25] lockfile: avoid transitory invalid states

Am 4/7/2014 13:13, schrieb Michael Haggerty:

On 04/07/2014 08:16 AM, Johannes Sixt wrote:

quoted

Am 4/7/2014 1:34, schrieb Michael Haggerty:

quoted

So, instead of encoding part of the lock_file state in the filename
field, add a new bit "LOCK_FLAGS_LOCKFILE_ACTIVE" to flags, and use
this bit to distinguish between a lock_file object that is active
vs. one that is inactive.  Be careful to set this bit only when
filename really contains the name of a file that should be deleted on
cleanup.

Since this flag is primarily for communication between the main code and a
signal handler, the only safe way is to define the flag as volatile
sig_atomic_t, not to make it a bit of a larger type!

Thanks for the feedback.  You are obviously right, and I will fix it.

But I have a feeling that this line of thought is going to lead to the
signal handler's not being able to do anything.  How far can we afford
to pursue strict correctness?  ...

The signal handler currently reads

    lock_file_list
    lock_file::next
    lock_file::fd
    lock_file::owner
    lock_file::filename
    *lock_file::filename

and writes lock_file_list.  Among other things it calls close(),
unlink(), vsnprintf(), and fprintf() (the last two via warning()).

But most of these actions are undefined under the C99 standard:

Good point. But not all is lost because some of the functions are
well-defined under POSIX, particularly close() and unlink(). (*printf are
not, though.)

I don't have time to rewrite *all* of Git right now, so how can we get
reasonable safety and portability within a feasible amount of work?

It shouldn't be *that* bad. We can make all members volatile, except
filename (because we wouldn't be able to strcpy(lk->filename, ...) without
a type cast).

How far *do* you want to go? I'm certainly not opposed to field-test your
current changeset (plus and adjustment to use sig_atomic_t) -- overall it
is an improvement. And then we will see how it works.

Just as food for thought: A compiler barrier should be sufficient to
inhibit that the compiler reorders code across accesses of the volatile
flag. Like in the main code:

	strcpy(lk->filename, ...);
	BARRIER();
	lk->is_active = 1;	/* volatile sig_atomic_t */

and in the signal handler:

	if (!lk->is_active)
		return;
	BARRIER();
	unlink(lk->filename);

with some suitable definition of BARRIER(). I don't think that we need an
explicit memory barrier (in practice) because that should be implied by
the context switch leading to the signal handler.

-- Hannes