git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 2/4] Correct comment in prepare_packed_git_one.
       [not found] <a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org>
@ 2007-02-01 20:52 ` Shawn O. Pearce
  2007-02-02  6:21   ` Junio C Hamano
  2007-02-01 20:52 ` [PATCH 3/4] Refactor open_packed_git to return an error code Shawn O. Pearce
  2007-02-01 20:52 ` [PATCH 4/4] Don't find objects in packs which aren't available anymore Shawn O. Pearce
  2 siblings, 1 reply; 4+ messages in thread
From: Shawn O. Pearce @ 2007-02-01 20:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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(-)

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 3/4] Refactor open_packed_git to return an error code.
       [not found] <a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org>
  2007-02-01 20:52 ` [PATCH 2/4] Correct comment in prepare_packed_git_one Shawn O. Pearce
@ 2007-02-01 20:52 ` Shawn O. Pearce
  2007-02-01 20:52 ` [PATCH 4/4] Don't find objects in packs which aren't available anymore Shawn O. Pearce
  2 siblings, 0 replies; 4+ messages in thread
From: Shawn O. Pearce @ 2007-02-01 20:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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(-)

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 4/4] Don't find objects in packs which aren't available anymore.
       [not found] <a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org>
  2007-02-01 20:52 ` [PATCH 2/4] Correct comment in prepare_packed_git_one Shawn O. Pearce
  2007-02-01 20:52 ` [PATCH 3/4] Refactor open_packed_git to return an error code Shawn O. Pearce
@ 2007-02-01 20:52 ` Shawn O. Pearce
  2 siblings, 0 replies; 4+ messages in thread
From: Shawn O. Pearce @ 2007-02-01 20:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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(-)

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/4] Correct comment in prepare_packed_git_one.
  2007-02-01 20:52 ` [PATCH 2/4] Correct comment in prepare_packed_git_one Shawn O. Pearce
@ 2007-02-02  6:21   ` Junio C Hamano
  0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2007-02-02  6:21 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: git

"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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2007-02-02  6:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <a0b03fc086bb66e2aa2e386dcb4ff97fc837f07f.1170363130.git.spearce@spearce.org>
2007-02-01 20:52 ` [PATCH 2/4] Correct comment in prepare_packed_git_one Shawn O. Pearce
2007-02-02  6:21   ` Junio C Hamano
2007-02-01 20:52 ` [PATCH 3/4] Refactor open_packed_git to return an error code Shawn O. Pearce
2007-02-01 20:52 ` [PATCH 4/4] Don't find objects in packs which aren't available anymore Shawn O. Pearce

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 NNTP newsgroup(s).