threads / patch / 6613

patch, 4 partsRefactor open_packed_git to return an error code.

Subject: [PATCH 3/4] Refactor open_packed_git to return an error code.

## tl;dr

4 messages between Feb 1, 2007 and Feb 2, 2007. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Shawn O. Pearce· Feb 1, 2007, 20:52 UTC · lore

[PATCH 2/4] Correct comment in prepare_packed_git_one.

After staring at the comment and the associated for loop, I realized the comment was completely bogus. The section of code its talking about is trying to avoid duplicate mapping of the same packfile.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to sha1_file.c +1 −1

diff --git a/sha1_file.c b/sha1_file.c
index a42f94a..4aef244 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -779,7 +779,7 @@ static void prepare_packed_git_one(char *objdir, int local)
 		if (!has_extension(de->d_name, ".idx"))
 			continue;
 
-		/* we have .idx.  Is it a file we can map? */
+		/* Don't reopen a pack we already have. */
 		strcpy(path + len, de->d_name);
 		for (p = packed_git; p; p = p->next) {
 			if (!memcmp(path, p->pack_name, len + namelen - 4))

-- 
1.5.0.rc3.1.ge4b0e

Junio C Hamano· Feb 2, 2007, 06:21 UTC · re: Shawn O. Pearce · lore

Re: [PATCH 2/4] Correct comment in prepare_packed_git_one.

"Shawn O. Pearce" <spearce@spearce.org> writes:

> After staring at the comment and the associated for loop, I
> realized the comment was completely bogus.

Yeah, when the comment was written, the loop after the comment did not exist. The comment was talking about calling add_packed_git() to see if the corresponding pack can be mapped, and then upon receiving NULL, not linking p to the packed_git linked list.

Shawn O. Pearce· Feb 1, 2007, 20:52 UTC · lore

Because I want to reuse open_packed_git in a context where I don't want the process to die if the packfile in question is bogus, I'm changing its behavior to return error("...") rather than die("...") when it detects something is wrong with the packfile it was given.

Right now we still must die out of use_pack should open_packed_git fail, as none of use_pack's callers are prepared to handle a failure from that function.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

Show changes to sha1_file.c +16 −15

diff --git a/sha1_file.c b/sha1_file.c
index 4aef244..277319b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -552,7 +552,7 @@ void unuse_pack(struct pack_window **w_cursor)
 	}
 }
 
-static void open_packed_git(struct packed_git *p)
+static int open_packed_git(struct packed_git *p)
 {
 	struct stat st;
 	struct pack_header hdr;
@@ -562,49 +562,50 @@ static void open_packed_git(struct packed_git *p)
 
 	p->pack_fd = open(p->pack_name, O_RDONLY);
 	if (p->pack_fd < 0 || fstat(p->pack_fd, &st))
-		die("packfile %s cannot be opened", p->pack_name);
+		return -1;
 
 	/* If we created the struct before we had the pack we lack size. */
 	if (!p->pack_size) {
 		if (!S_ISREG(st.st_mode))
-			die("packfile %s not a regular file", p->pack_name);
+			return error("packfile %s not a regular file", p->pack_name);
 		p->pack_size = st.st_size;
 	} else if (p->pack_size != st.st_size)
-		die("packfile %s size changed", p->pack_name);
+		return error("packfile %s size changed", p->pack_name);
 
 	/* We leave these file descriptors open with sliding mmap;
 	 * there is no point keeping them open across exec(), though.
 	 */
 	fd_flag = fcntl(p->pack_fd, F_GETFD, 0);
 	if (fd_flag < 0)
-		die("cannot determine file descriptor flags");
+		return error("cannot determine file descriptor flags");
 	fd_flag |= FD_CLOEXEC;
 	if (fcntl(p->pack_fd, F_SETFD, fd_flag) == -1)
-		die("cannot set FD_CLOEXEC");
+		return error("cannot set FD_CLOEXEC");
 
 	/* Verify we recognize this pack file format. */
 	if (read_in_full(p->pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
-		die("file %s is far too short to be a packfile", p->pack_name);
+		return error("file %s is far too short to be a packfile", p->pack_name);
 	if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
-		die("file %s is not a GIT packfile", p->pack_name);
+		return error("file %s is not a GIT packfile", p->pack_name);
 	if (!pack_version_ok(hdr.hdr_version))
-		die("packfile %s is version %u and not supported"
+		return error("packfile %s is version %u and not supported"
 			" (try upgrading GIT to a newer version)",
 			p->pack_name, ntohl(hdr.hdr_version));
 
 	/* Verify the pack matches its index. */
 	if (num_packed_objects(p) != ntohl(hdr.hdr_entries))
-		die("packfile %s claims to have %u objects"
+		return error("packfile %s claims to have %u objects"
 			" while index size indicates %u objects",
 			p->pack_name, ntohl(hdr.hdr_entries),
 			num_packed_objects(p));
 	if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
-		die("end of packfile %s is unavailable", p->pack_name);
+		return error("end of packfile %s is unavailable", p->pack_name);
 	if (read_in_full(p->pack_fd, sha1, sizeof(sha1)) != sizeof(sha1))
-		die("packfile %s signature is unavailable", p->pack_name);
+		return error("packfile %s signature is unavailable", p->pack_name);
 	idx_sha1 = ((unsigned char *)p->index_base) + p->index_size - 40;
 	if (hashcmp(sha1, idx_sha1))
-		die("packfile %s does not match index", p->pack_name);
+		return error("packfile %s does not match index", p->pack_name);
+	return 0;
 }
 
 static int in_window(struct pack_window *win, unsigned long offset)
@@ -627,8 +628,8 @@ unsigned char* use_pack(struct packed_git *p,
 {
 	struct pack_window *win = *w_cursor;
 
-	if (p->pack_fd == -1)
-		open_packed_git(p);
+	if (p->pack_fd == -1 && open_packed_git(p))
+		die("packfile %s cannot be accessed", p->pack_name);
 
 	/* Since packfiles end in a hash of their content and its
 	 * pointless to ask for an offset into the middle of that

-- 
1.5.0.rc3.1.ge4b0e

Shawn O. Pearce· Feb 1, 2007, 20:52 UTC · lore

[PATCH 4/4] Don't find objects in packs which aren't available anymore.

Matthias Lederhofer identified a race condition where a Git reader process was able to locate an object in a packed_git index, but was then preempted while a `git repack -a -d` ran and completed. By the time the reader was able to seek in the packfile to get the object data, the packfile no longer existed on disk.

In this particular case the reader process did not attempt to open the packfile before it was deleted, so it did not already have the pack_fd field popuplated. With the packfile itself gone, there was no way for the reader to open it and fetch the data.

I'm fixing the race condition by teaching find_pack_entry to ignore a packed_git whose packfile is not currently open and which cannot be opened. If none of the currently known packs can supply the object, we will return 0 and the caller will decide the object is not available. If this is the first attempt at finding an object, the caller will reprepare_packed_git and try again. If it was the second attempt, the caller will typically return NULL back, and an error message about a missing object will be reported.

This patch does not address the situation of a reader which is being starved out by a tight sequence of `git repack -a -d` runs. In this particular case the reader will try twice, probably fail both times, and declare the object in question cannot be found. As it is highly unlikely that a real world `git repack -a -d` can complete faster than a reader can open a packfile, so I don't think this is a huge concern.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 sha1_file.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Show changes to sha1_file.c +12 −0

diff --git a/sha1_file.c b/sha1_file.c
index 277319b..37669d6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1405,6 +1405,18 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons
 		}
 		offset = find_pack_entry_one(sha1, p);
 		if (offset) {
+			/*
+			 * We are about to tell the caller where they can
+			 * locate the requested object.  We better make
+			 * sure the packfile is still here and can be
+			 * accessed before supplying that answer, as
+			 * it may have been deleted since the index
+			 * was loaded!
+			 */
+			if (p->pack_fd == -1 && open_packed_git(p)) {
+				error("packfile %s cannot be accessed", p->pack_name);
+				continue;
+			}
 			e->offset = offset;
 			e->p = p;
 			hashcpy(e->sha1, sha1);

-- 
1.5.0.rc3.1.ge4b0e