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

Daniel Barkalow [off-list ref] 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 <redacted>
---
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(-)

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;

Hi,

On Sat, 19 Apr 2008, Junio C Hamano wrote:

Daniel Barkalow [off-list ref] writes:

quoted

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 <redacted>
---
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

On Sat, 19 Apr 2008, Junio C Hamano wrote:

Daniel Barkalow [off-list ref] writes:

quoted

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 <redacted>
---
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*