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

[Johannes Sixt <j.sixt@viscovery.net>]

Am 4/7/2014 13:13, schrieb Michael Haggerty:
> On 04/07/2014 08:16 AM, Johannes Sixt wrote:
>> Am 4/7/2014 1:34, schrieb Michael Haggerty:
>>> 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