# [PATCH 2/8] Add a lockfile function to append to a file 4 messages from 2008-04-18 to 2008-04-20. Participants: Daniel Barkalow, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/13168 ## Daniel Barkalow, 2008-04-18 00:02 Subject: [PATCH 2/8] Add a lockfile function to append to a file Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0804171950140.19665%40iabervon.org ``` 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 --- 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(-) 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, 2008-04-20 04:59 Subject: Re: [PATCH 2/8] Add a lockfile function to append to a file Message-ID: <7vtzhxce9g.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtzhxce9g.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow 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 > --- > 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; ``` ## Johannes Schindelin, 2008-04-20 16:14 Subject: Re: [PATCH 2/8] Add a lockfile function to append to a file Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0804201714230.5084%40eeepc-johanness In-Reply-To: <7vtzhxce9g.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sat, 19 Apr 2008, Junio C Hamano wrote: > Daniel Barkalow 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 > > --- > > 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, 2008-04-20 19:43 Subject: Re: [PATCH 2/8] Add a lockfile function to append to a file Message-ID: URL: https://gitlist.dev/e/alpine.LNX.1.00.0804201528350.19665%40iabervon.org In-Reply-To: <7vtzhxce9g.fsf@gitster.siamese.dyndns.org> ``` On Sat, 19 Apr 2008, Junio C Hamano wrote: > Daniel Barkalow 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 > > --- > > 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* ```