git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Johannes.Schindelin@gmx.de,
	dstolee@microsoft.com, jeffhost@microsoft.com, peff@peff.net
Subject: Re: [PATCH 4/4] midx.c: guard against commit_lock_file() failures
Date: Sat, 09 Oct 2021 04:07:16 +0200	[thread overview]
Message-ID: <875yu7lzkp.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <ed7407cefa89b973d1085973f4ddb39397597036.1633729502.git.me@ttaylorr.com>


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.

  reply	other threads:[~2021-10-09  2:08 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-08 21:46 [PATCH 0/4] midx: avoid potential rename-while-mapped on Windows Taylor Blau
2021-10-08 21:46 ` [PATCH 1/4] midx.c: extract MIDX lookup by object_dir Taylor Blau
2021-10-08 21:46 ` [PATCH 2/4] midx.c: lookup MIDX by object directory during expire Taylor Blau
2021-10-13 13:28   ` Derrick Stolee
2021-10-08 21:46 ` [PATCH 3/4] midx.c: lookup MIDX by object directory during repack Taylor Blau
2021-10-08 21:46 ` [PATCH 4/4] midx.c: guard against commit_lock_file() failures Taylor Blau
2021-10-09  2:07   ` Ævar Arnfjörð Bjarmason [this message]
2021-10-13 13:29 ` [PATCH 0/4] midx: avoid potential rename-while-mapped on Windows Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875yu7lzkp.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=jeffhost@microsoft.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).