threads / patch / 13168

patch, 8 partsAdd a lockfile function to append to a file

Subject: [PATCH 2/8] Add a lockfile function to append to a file

## tl;dr

4 messages between Apr 18, 2008 and Apr 20, 2008. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Daniel Barkalow· Apr 18, 2008, 00:02 UTC · lore

This takes care of copying the original contents into the replacement file after the lock is held, so that concurrent additions can't miss each other's changes.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
How about this? Also doesn't leak a fd and catches trying to append to a 
file you can't read. Should I worry about mmap failing after the open?
 cache.h    |    1 +
 lockfile.c |   23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)
Show changes to 2 files +24 −0

cache.h, lockfile.c

diff --git a/cache.h b/cache.h
index 50b28fa..8d066bf 100644
--- a/cache.h
+++ b/cache.h
@@ -391,6 +391,7 @@ struct lock_file {
 	char filename[PATH_MAX];
 };
 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 commit_lock_file(struct lock_file *);
 
 extern int hold_locked_index(struct lock_file *, int);
diff --git a/lockfile.c b/lockfile.c
index 663f18f..f1b5416 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -160,6 +160,29 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
 	return fd;
 }
 
+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)
+{
+	int fd = lock_file(lk, path);
+	struct stat st;
+	if (fd < 0) {
+		if (die_on_error)
+			die("unable to create '%s.lock': %s",
+			    path, strerror(errno));
+	} else if (!stat(path, &st)) {
+		int orig_fd = open(path, O_RDONLY);
+		size_t mmap_size = xsize_t(st.st_size);
+		void *mmap;
+		if (orig_fd < 0)
+			die("unable to read %s to append to it", path);
+		mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE,
+				   orig_fd, 0);
+		write_or_die(fd, mmap, mmap_size);
+		munmap(mmap, mmap_size);
+		close(orig_fd);
+	}
+	return fd;
+}
+
 int close_lock_file(struct lock_file *lk)
 {
 	int fd = lk->fd;

-- 
1.5.4.3.610.gea6cd
Junio C Hamano· Apr 20, 2008, 04:59 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/8] Add a lockfile function to append to a file

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 8 quoted lines
> This takes care of copying the original contents into the replacement
> file after the lock is held, so that concurrent additions can't miss
> each other's changes.
>
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---
> How about this? Also doesn't leak a fd and catches trying to append to a 
> file you can't read. Should I worry about mmap failing after the open?

We have copy.c to copy small existing files, while detecting failure to copy properly. How about doing something like this instead?

 cache.h    |    1 +
 lockfile.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
Show changes to 2 files +29 −0

cache.h, lockfile.c

diff --git a/cache.h b/cache.h
index 50b28fa..8d066bf 100644
--- a/cache.h
+++ b/cache.h
@@ -391,6 +391,7 @@ struct lock_file {
 	char filename[PATH_MAX];
 };
 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 commit_lock_file(struct lock_file *);
 
 extern int hold_locked_index(struct lock_file *, int);
diff --git a/lockfile.c b/lockfile.c
index 663f18f..e9e0095 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -160,6 +160,34 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
 	return fd;
 }
 
+int hold_lock_file_for_append(struct lock_file *lk, const char *path, int die_on_error)
+{
+	int fd, orig_fd;
+
+	fd = lock_file(lk, path);
+	if (fd < 0) {
+		if (die_on_error)
+			die("unable to create '%s.lock': %s", path, strerror(errno));
+		return fd;
+	}
+
+	orig_fd = open(path, O_RDONLY);
+	if (orig_fd < 0) {
+		if (errno != ENOENT) {
+			if (die_on_error)
+				die("cannot open '%s' for copying", path);
+			close(fd);
+			return error("cannot open '%s' for copying", path);
+		}
+	} else if (copy_fd(orig_fd, fd)) {
+		if (die_on_error)
+			exit(128);
+		close(fd);
+		return -1;
+	}
+	return fd;
+}
+
 int close_lock_file(struct lock_file *lk)
 {
 	int fd = lk->fd;
Johannes Schindelin· Apr 20, 2008, 16:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/8] Add a lockfile function to append to a file

Hi,
On Sat, 19 Apr 2008, Junio C Hamano wrote:
Show 13 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > This takes care of copying the original contents into the replacement
> > file after the lock is held, so that concurrent additions can't miss
> > each other's changes.
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > How about this? Also doesn't leak a fd and catches trying to append to a 
> > file you can't read. Should I worry about mmap failing after the open?
> 
> We have copy.c to copy small existing files, while detecting failure to
> copy properly.  How about doing something like this instead?
I like it.

Ciao, Dscho

Daniel Barkalow· Apr 20, 2008, 19:43 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/8] Add a lockfile function to append to a file

On Sat, 19 Apr 2008, Junio C Hamano wrote:
Show 13 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > This takes care of copying the original contents into the replacement
> > file after the lock is held, so that concurrent additions can't miss
> > each other's changes.
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > How about this? Also doesn't leak a fd and catches trying to append to a 
> > file you can't read. Should I worry about mmap failing after the open?
> 
> We have copy.c to copy small existing files, while detecting failure to
> copy properly.  How about doing something like this instead?
Yeah, that's obviously the right thing to use.
	-Daniel
*This .sig left intentionally blank*

← back to recent threads