Re: [PATCH v5] lockfile: add PID file for debugging stale locks
- From
Jeff King <peff@peff.net>
- Date
- Jan 21, 2026, 07:13 UTC
- Message-ID
- <20260121071344.GA570838@coredump.intra.peff.net>
- In-Reply-To
- <pull.2011.v5.git.1768933954845.gitgitgadget@gmail.com>
On Tue, Jan 20, 2026 at 06:32:34PM +0000, Paulo Casaretto via GitGitGadget wrote:
Show 32 quoted lines
> +static struct tempfile *create_lock_pid_file(const char *pid_path, int mode)
> +{
> + struct strbuf content = STRBUF_INIT;
> + struct tempfile *pid_tempfile = NULL;
> + int fd = -1;
> +
> + if (!lockfile_pid_enabled)
> + goto out;
> +
> + fd = open(pid_path, O_WRONLY | O_CREAT | O_EXCL, mode);
> + if (fd < 0)
> + goto out;
> +
> + strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid());
> + if (write_in_full(fd, content.buf, content.len) < 0) {
> + warning_errno(_("could not write lock pid file '%s'"), pid_path);
> + close(fd);
> + fd = -1;
> + unlink(pid_path);
> + goto out;
> + }
> +
> + close(fd);
> + fd = -1;
> + pid_tempfile = register_tempfile(pid_path);
> +
> +out:
> + if (fd >= 0)
> + close(fd);
> + strbuf_release(&content);
> + return pid_tempfile;
> +}Coverity complains that the close(fd) call in the "out" label is unreachable, and I think it is right. When we jump from before the open(), or if the open failed, then fd is negative (and thus no close). If we get there when write_in_full() fails, then we close ourselves in the conditional. And if we succeed, then we close the descriptor before registering the tempfile.
I don't think it's wrong, but the cleanup is redundant between the "out" path and the others.
Did you mean this:
diff --git a/lockfile.c b/lockfile.c index 731cdd4944..e5d6ae0df6 100644 --- a/lockfile.c +++ b/lockfile.c @@ -122,14 +122,10 @@ static struct tempfile *create_lock_pid_file(const char *pid_path, int mode) strbuf_addf(&content, "pid %" PRIuMAX "\n", (uintmax_t)getpid()); if (write_in_full(fd, content.buf, content.len) < 0) { warning_errno(_("could not write lock pid file '%s'"), pid_path); - close(fd); - fd = -1; unlink(pid_path); goto out; } - close(fd); - fd = -1; pid_tempfile = register_tempfile(pid_path); out: which would just let the close after the out label handle all cases? -Peff