Re: [PATCH 4/5] lock_file: make function-local locks non-static
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- May 7, 2018, 21:19 UTC
- Message-ID
- <CAN0heSr20XFj=W28_kZAp9BWWHExQGMYa4Om=ieYWw+ucrLhRA@mail.gmail.com>
- In-Reply-To
- <CACsJy8DDyrUinwXx1b66DCHB+2DLt1KBmFt_83R1+HWjbzGH2A@mail.gmail.com>
On 7 May 2018 at 17:24, Duy Nguyen <pclouds@gmail.com> wrote:
Show 26 quoted lines
> 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: >> 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.
Right, that could also happen.
Show 14 quoted lines
>> 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).
One thing I can try to make clearer is that no-one is holding on to the address of a `struct lock_file`. Not anyone, anywhere. So whether that location gets filled with random junk after the function has returned is irrelevant when it comes to the eventual cleaning up. The only thing that anyone is keeping track of is the *temp*file. The `struct lock_file` is just a fancy wrapper for keeping a pointer to a tempfile. The tempfile infrastructure keeps track of the tempfiles directly, without tracking any lockfiles.
Maybe it is a bit optimistic to tackle all of these at one fell swoop. Fiddling with lockfiles makes people nervous, for good reasons. I could go for the ones where it is easy to see that we always clean things up, then handle the less trivial ones in a second run-over, or not at all.
Martin