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

On 04/07/2014 02:12 PM, Johannes Sixt wrote:

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

quoted

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.)

quoted

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.

For now I think I'd just like to get the biggest problems fixed without
making anything worse.  Given that there might be a GSoC student working
in this neighborhood, he/she might be able to take up the baton.

I changed the patch series to use a new "volatile sig_atomic_t active"
field rather than a bit in a "flags" field.  I'll wait a short time to
see if there is more feedback before pushing it to the list; meanwhile
you can find it here if you have time to look at it and/or test it:

    http://github.com/mhagger/git, branch "lock-correctness"

Michael


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/