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@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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