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@xxxxxxxxxxxx -- 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