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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html