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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 04/07/2014 02:12 PM, Johannes Sixt wrote:
> 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.

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/