Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back

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

On 09/17/2014 12:19 AM, Jonathan Nieder wrote:
> Michael Haggerty wrote:
> 
>> If closing an open lockfile fails, then we cannot be sure of the
>> contents of the lockfile
> 
> Is that true?  It seems more like a bug in close_lock_file: if it
> fails, perhaps it should either set lk->fd back to fd or unlink the
> lockfile itself.

>From close(2) (note especially the last sentence):

> Not checking the return value of close() is a common but nevertheless serious  programming
> error.   It  is  quite  possible  that  errors  on a previous write(2) operation are first
> reported at the final close().  Not checking the return value when closing  the  file  may
> lead  to  silent  loss  of  data.   This can especially be observed with NFS and with disk
> quota.  Note that the return value should only be used  for  diagnostics.   In  particular
> close() should not be retried after an EINTR since this may cause a reused descriptor from
> another thread to be closed.

>From that I conclude that if close() fails, pretty much all bets are off
about the contents of the lockfile. So we wouldn't want to set lk->fd
back to fd.

But finishing the rollback itself might be an alternative...

> What do other callers do on close_lock_file failure?

>From what I can see, the only callers that don't die() immediately are
the following (which call close_lock_file() directly or indirectly via
write_locked_index()):

try_merge_strategy(): returns an error. It looks like this could end up
reusing the same lock_file object before it had been rolled back ->
would be improved if close_lock_file() would rollback on failure.

write_cache_as_tree(): does a rollback -> wouldn't mind an automatic
rollback.

merge_recursive_generic(): returns an error, and caller exits
immediately -> wouldn't mind an automatic rollback.

So, I will change close_lock_file() to roll back on errors.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu