This way, the code does not need to carefully safeguard errno to allow
callers to print a reasonable error message when they choose to do
some cleanup before die()-ing.
Fixes a bug waiting to happen where copy_fd would clobber the errno
passed back via hold_lock_file_for_append from read() or write() when
flags did not contain LOCK_DIE_ON_ERROR. Luckily the only caller uses
flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.
Reported-by: Ronnie Sahlberg <sahlb...@google.com>
Signed-off-by: Jonathan Nieder <jrnie...@gmail.com>
---
The advertised bugfix.
lockfile.c | 29 ++++++++++-------------------
lockfile.h | 3 ++-
sha1_file.c | 7 ++++++-
3 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index b3020f3..8685c68 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
return fd;
}
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int
flags)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+ int flags, struct strbuf *err)
{
int fd, orig_fd;
- struct strbuf err = STRBUF_INIT;
+
+ assert(!(flags & LOCK_DIE_ON_ERROR));
+ assert(err && !err->len);
fd = lock_file(lk, path, flags);
if (fd < 0) {
- if (flags & LOCK_DIE_ON_ERROR)
- unable_to_lock_die(path, errno);
+ unable_to_lock_message(path, errno, err);
return fd;
}
orig_fd = open(path, O_RDONLY);
if (orig_fd < 0) {
if (errno != ENOENT) {
- int save_errno = errno;
-
- if (flags & LOCK_DIE_ON_ERROR)
- die("cannot open '%s' for copying", path);
+ strbuf_addf(err, "cannot open '%s' for copying: %s",
+ path, strerror(errno));
rollback_lock_file(lk);
- error("cannot open '%s' for copying", path);
- errno = save_errno;
return -1;
}
- } else if (copy_fd(orig_fd, fd, &err)) {
- int save_errno = errno;
-
- error("copy-fd: %s", err.buf);
- strbuf_release(&err);
- if (flags & LOCK_DIE_ON_ERROR)
- exit(128);
+ } else if (copy_fd(orig_fd, fd, err)) {
+ strbuf_prefixf(err, "cannot copy '%s': ", path);
close(orig_fd);
rollback_lock_file(lk);
- errno = save_errno;
return -1;
} else {
close(orig_fd);
}
- strbuf_release(&err);
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index cd2ec95..ca36a1d 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
-extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
+extern int hold_lock_file_for_append(struct lock_file *, const char *path,
+ int, struct strbuf *err);
extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
extern char *get_locked_file_path(struct lock_file *);
extern int commit_lock_file_to(struct lock_file *, const char *path);
diff --git a/sha1_file.c b/sha1_file.c
index e1945e2..6c0ab3b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -406,17 +406,22 @@ void add_to_alternates_file(const char *reference)
struct lock_file *lock;
int fd;
char *alt;
+ struct strbuf err = STRBUF_INIT;
lock = xcalloc(1, sizeof(*lock));
fd = hold_lock_file_for_append(lock, mkpath("%s/info/alternates",
get_object_directory()),
- LOCK_DIE_ON_ERROR);
+ 0, &err);
+ if (fd < 0)
+ die("%s", err.buf);
alt = mkpath("%s\n", reference);
write_or_die(fd, alt, strlen(alt));
if (commit_lock_file(lock))
die("could not close alternates file");
if (alt_odb_tail)
link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
+
+ strbuf_release(&err);
}
int foreach_alt_odb(alt_odb_fn fn, void *cb)
--
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
