Re: [PATCH 4/5] lock_file: make function-local locks non-static

[Duy Nguyen <pclouds@gmail.com>]

On Sun, May 6, 2018 at 9:32 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 6 May 2018 at 19:42, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>>>> These `struct lock_file`s are local to their respective functions and we
>>>> can drop their staticness.
>
>>>> -       static struct lock_file lock;
>>>> +       struct lock_file lock = LOCK_INIT;
>>>
>>> Is it really safe to do this? I vaguely remember something about
>>> (global) linked list and signal handling which could trigger any time
>>> and probably at atexit() time too (i.e. die()). You don't want to
>>> depend on stack-based variables in that case.
>>
>> So I dug in a bit more about this. The original implementation does
>> not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
>> Implement git-checkout-cache -u to update stat information in the
>> cache. - 2005-05-15). The situation has changed since 422a21c6a0
>> (tempfile: remove deactivated list entries - 2017-09-05). At the end
>> of that second commit, Jeff mentioned "We can clean them up
>> individually" which I guess is what these patches do. Though I do not
>> know if we need to make sure to call "release" function or something/
>> Either way you need more explanation and assurance than just "we can
>> drop their staticness" in the commit mesage.
>
> Thank you Duy for your comments. How about I write the commit message
> like so:

+Jeff. Since he made it possible to remove lock file from the global
linked list, he probably knows well what to check when switching from
a static lock file to a stack-local one.

>
>   After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05),
>   we can have lockfiles on the stack. These `struct lock_file`s are local
>   to their respective functions and we can drop their staticness.
>
>   Each of these users either commits or rolls back the lock in every
>   codepath, with these possible exceptions:
>
>     * We bail using a call to `die()` or `exit()`. The lock will be
>       cleaned up automatically.
>
>     * We return early from a function `cmd_foo()` in builtin/, i.e., we
>       are just about to exit. The lock will be cleaned up automatically.

There are also signals which can be caught and run on its own stack (I
think) so whatever variable on the current stack should be safe, I
guess.

>   If I have missed some codepath where we do not exit, yet leave a locked
>   lock around, that was so also before this patch. If we would later
>   re-enter the same function, then before this patch, we would be retaking
>   a lock for the very same `struct lock_file`, which feels awkward, but to
>   the best of my reading has well-defined behavior. Whereas after this
>   patch, we would attempt to take the lock with a completely fresh `struct
>   lock_file`. In both cases, the result would simply be that the lock can
>   not be taken, which is a situation we already handle.

There is a difference here, if the lock is not released properly,
previously the lockfile is still untouched. If it's on stack, it may
be overwritten which can corrupt the linked list to get to the next
lock file.  (and this is about calling the function in question just
_once_ not the second time).
-- 
Duy