Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back
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 mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 18/35] commit_lock_file(): if close fails, roll back
If closing an open lockfile fails, then we cannot be sure of the contents of the lockfile, so there is nothing sensible to do but delete it. This change also leaves the lock_file object in a defined state in this error path (namely, unlocked). Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- lockfile.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index 6436111..becb3da 100644 --- a/lockfile.c +++ b/lockfile.c @@ -315,8 +315,12 @@ int commit_lock_file(struct lock_file *lk) if (!lk-filename[0]) die(BUG: attempt to commit unlocked object); - if (lk-fd = 0 close_lock_file(lk)) + if (lk-fd = 0 close_lock_file(lk)) { + int save_errno = errno; + rollback_lock_file(lk); + errno = save_errno; return -1; + } strcpy(result_file, lk-filename); /* remove .lock: */ -- 2.1.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 18/35] commit_lock_file(): if close fails, roll back
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. What do other callers do on close_lock_file failure? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html