Re: [PATCH v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP
On 04/07/2014 09:31 PM, Jeff King wrote: On Mon, Apr 07, 2014 at 01:33:59AM +0200, Michael Haggerty wrote: It was previously a bug to call commit_lock_file() with a lock_file object that was not active (an illegal access would happen within the function). It was presumably never done, but this would be an easy programming error to overlook. So guard the file-renaming code with an if statement to change committing an unlocked file into a NOP. [...] Yeah, I would have expected a die(BUG) here. I think it is worth making it a fatal mistake and catching it. Rolling back an uninitialized lockfile is probably OK; we are canceling an operation that never started. But committing a lockfile that we didn't actually fill out could be a sign of a serious error, and we may be propagating a bogus success code. E.g., imagine that receive-pack claims to have written your ref, but actually commit_lock_file was a silent NOP. I'd much rather have it die loudly so we can track down the case. OK, I will change this to a fatal error in v3. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP
On Mon, Apr 07, 2014 at 01:33:59AM +0200, Michael Haggerty wrote: It was previously a bug to call commit_lock_file() with a lock_file object that was not active (an illegal access would happen within the function). It was presumably never done, but this would be an easy programming error to overlook. So guard the file-renaming code with an if statement to change committing an unlocked file into a NOP. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- Alternatively, we could make it a fatal error (e.g., an assert() or if...die()) to call commit_lock_file() on an unlocked file, or omit a warning in this case. But since it is so hard to test code related to locking failures, I have the feeling that such an error is most likely to occur in some error-handling path, maybe when some other lockfile acquisition has failed, and it would be better to let the code continue its attempted cleanup instead of dying. But it would be easy to persuade me to change my opinion. Yeah, I would have expected a die(BUG) here. I think it is worth making it a fatal mistake and catching it. Rolling back an uninitialized lockfile is probably OK; we are canceling an operation that never started. But committing a lockfile that we didn't actually fill out could be a sign of a serious error, and we may be propagating a bogus success code. E.g., imagine that receive-pack claims to have written your ref, but actually commit_lock_file was a silent NOP. I'd much rather have it die loudly so we can track down the case. -Peff -- 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 v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP
It was previously a bug to call commit_lock_file() with a lock_file object that was not active (an illegal access would happen within the function). It was presumably never done, but this would be an easy programming error to overlook. So guard the file-renaming code with an if statement to change committing an unlocked file into a NOP. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- Alternatively, we could make it a fatal error (e.g., an assert() or if...die()) to call commit_lock_file() on an unlocked file, or omit a warning in this case. But since it is so hard to test code related to locking failures, I have the feeling that such an error is most likely to occur in some error-handling path, maybe when some other lockfile acquisition has failed, and it would be better to let the code continue its attempted cleanup instead of dying. But it would be easy to persuade me to change my opinion. Documentation/technical/api-lockfile.txt | 4 +++- lockfile.c | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index b53e300..bfb2676 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -68,7 +68,9 @@ commit_lock_file:: with an earlier call to `hold_lock_file_for_update()`, close the file descriptor and rename the lockfile to its final destination. Returns 0 upon success, a negative - value on failure to close(2) or rename(2). + value on failure to close(2) or rename(2). It is a NOP to + call `commit_lock_file()` for a `lock_file` object that is not + currently locked. rollback_lock_file:: diff --git a/lockfile.c b/lockfile.c index 47762c6..24aaa53 100644 --- a/lockfile.c +++ b/lockfile.c @@ -301,6 +301,9 @@ int commit_lock_file(struct lock_file *lk) if (lk-fd = 0 close_lock_file(lk)) return -1; + if (!lk-filename[0]) + return 0; + strcpy(result_file, lk-filename); /* remove .lock: */ result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0; -- 1.9.1 -- 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
