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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
