| Message ID | ed7407cefa89b973d1085973f4ddb39397597036.1633729502.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | midx: avoid potential rename-while-mapped on Windows | expand |
On Fri, Oct 08 2021, Taylor Blau wrote: > When writing a MIDX, we atomically move the new MIDX into place via > commit_lock_file(), but do not check to see if that call was successful. > > Make sure that we do check in order to prevent us from incorrectly > reporting that we wrote a new MIDX if we actually encountered an error. > > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > midx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/midx.c b/midx.c > index 137af3fc67..e79fb4427d 100644 > --- a/midx.c > +++ b/midx.c > @@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir, > if (ctx.m) > close_object_store(the_repository->objects); > > - commit_lock_file(&lk); > + if (commit_lock_file(&lk) < 0) > + die_errno(_("could not write multi-pack-index")); > > clear_midx_files_ext(object_dir, ".bitmap", midx_hash); > clear_midx_files_ext(object_dir, ".rev", midx_hash); As an aside I've wondered with this API whether something like this wouldn't make sense (obviously with a better message): diff --git a/lockfile.c b/lockfile.c index cc9a4b84283..6632a634f00 100644 --- a/lockfile.c +++ b/lockfile.c @@ -175,6 +175,7 @@ int hold_lock_file_for_update_timeout_mode(struct lock_file *lk, long timeout_ms, int mode) { int fd = lock_file_timeout(lk, path, flags, timeout_ms, mode); + lk->flags = flags; if (fd < 0) { if (flags & LOCK_DIE_ON_ERROR) unable_to_lock_die(path, errno); @@ -209,6 +210,8 @@ int commit_lock_file(struct lock_file *lk) int save_errno = errno; free(result_path); errno = save_errno; + if (lk->flags & LOCK_DIE_ON_ERROR) + die_errno("noes"); return -1; } free(result_path); diff --git a/lockfile.h b/lockfile.h index db93e6ba73e..40a9d91a6c5 100644 --- a/lockfile.h +++ b/lockfile.h @@ -119,6 +119,7 @@ struct lock_file { struct tempfile *tempfile; + int flags; }; #define LOCK_INIT { NULL } But a quick grep of the users reveals that some die on lock failure, but are happy to ignore commit_lock_file() failures, or maybe those are also bugs similar to the one you're fixing here & we could fix the API more generally.
diff --git a/midx.c b/midx.c index 137af3fc67..e79fb4427d 100644 --- a/midx.c +++ b/midx.c @@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir, if (ctx.m) close_object_store(the_repository->objects); - commit_lock_file(&lk); + if (commit_lock_file(&lk) < 0) + die_errno(_("could not write multi-pack-index")); clear_midx_files_ext(object_dir, ".bitmap", midx_hash); clear_midx_files_ext(object_dir, ".rev", midx_hash);
When writing a MIDX, we atomically move the new MIDX into place via commit_lock_file(), but do not check to see if that call was successful. Make sure that we do check in order to prevent us from incorrectly reporting that we wrote a new MIDX if we actually encountered an error. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)