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 [email protected] -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
