From 013870cd2cb1b0d6719a7a9123e126a62426520b Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 13:14:47 +0200 Subject: [PATCH 1/3] fdopen_lock_file(): access a lockfile using stdio Add a new function, fdopen_lock_file(), which returns a FILE pointer open to the lockfile. If a stream is open on a lock_file object, it is closed using fclose() on commit, rollback, or close_lock_file(). This change will allow callers to use stdio to write to a lockfile without having to muck around in the internal representation of the lock_file object (callers will be rewritten in upcoming commits). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 34 ++++++++++++------ lockfile.c | 46 +++++++++++++++++++++--- lockfile.h | 4 +++ 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index d4484d154d..93b5f23e4c 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -42,9 +42,13 @@ The caller: of the final destination (e.g. `$GIT_DIR/index`) to `hold_lock_file_for_update` or `hold_lock_file_for_append`. -* Writes new content for the destination file by writing to the file - descriptor returned by those functions (also available via - `lock->fd`). +* Writes new content for the destination file by either: + + * writing to the file descriptor returned by the `hold_lock_file_*` + functions (also available via `lock->fd`). + + * calling `fdopen_lock_file` to get a `FILE` pointer for the open + file and writing to the file using stdio. When finished writing, the caller can: @@ -70,10 +74,10 @@ any uncommitted changes. If you need to close the file descriptor you obtained from a `hold_lock_file_*` function yourself, do so by calling -`close_lock_file`. You should never call `close(2)` yourself! -Otherwise the `struct lock_file` structure would still think that the -file descriptor needs to be closed, and a commit or rollback would -result in duplicate calls to `close(2)`. Worse yet, if you `close(2)` +`close_lock_file`. You should never call `close(2)` or `fclose(3)` +yourself! Otherwise the `struct lock_file` structure would still think +that the file descriptor needs to be closed, and a commit or rollback +would result in duplicate calls to `close(2)`. Worse yet, if you close and then later open another file descriptor for a completely different purpose, then a commit or rollback might close that unrelated file descriptor. @@ -143,6 +147,13 @@ hold_lock_file_for_append:: the existing contents of the file (if any) to the lockfile and position its write pointer at the end of the file. +fdopen_lock_file:: + + Associate a stdio stream with the lockfile. Return NULL + (*without* rolling back the lockfile) on error. The stream is + closed automatically when `close_lock_file` is called or when + the file is committed or rolled back. + get_locked_file_path:: Return the path of the file that is locked by the specified @@ -179,10 +190,11 @@ close_lock_file:: Take a pointer to the `struct lock_file` initialized with an earlier call to `hold_lock_file_for_update` or - `hold_lock_file_for_append`, and close the file descriptor. - Return 0 upon success. On failure to `close(2)`, return a - negative value and roll back the lock file. Usually - `commit_lock_file`, `commit_lock_file_to`, or + `hold_lock_file_for_append`. Close the file descriptor (and + the file pointer if it has been opened using + `fdopen_lock_file`). Return 0 upon success. On failure to + `close(2)`, return a negative value and roll back the lock + file. Usually `commit_lock_file`, `commit_lock_file_to`, or `rollback_lock_file` should eventually be called if `close_lock_file` succeeds. diff --git a/lockfile.c b/lockfile.c index d27e61cafc..e0460275e1 100644 --- a/lockfile.c +++ b/lockfile.c @@ -7,20 +7,29 @@ static struct lock_file *volatile lock_file_list; -static void remove_lock_files(void) +static void remove_lock_files(int skip_fclose) { pid_t me = getpid(); while (lock_file_list) { - if (lock_file_list->owner == me) + if (lock_file_list->owner == me) { + /* fclose() is not safe to call in a signal handler */ + if (skip_fclose) + lock_file_list->fp = NULL; rollback_lock_file(lock_file_list); + } lock_file_list = lock_file_list->next; } } +static void remove_lock_files_on_exit(void) +{ + remove_lock_files(0); +} + static void remove_lock_files_on_signal(int signo) { - remove_lock_files(); + remove_lock_files(1); sigchain_pop(signo); raise(signo); } @@ -97,7 +106,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) if (!lock_file_list) { /* One-time initialization */ sigchain_push_common(remove_lock_files_on_signal); - atexit(remove_lock_files); + atexit(remove_lock_files_on_exit); } if (lk->active) @@ -106,6 +115,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) if (!lk->on_list) { /* Initialize *lk and add it to lock_file_list: */ lk->fd = -1; + lk->fp = NULL; lk->active = 0; lk->owner = 0; strbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN); @@ -214,6 +224,17 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) return fd; } +FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) +{ + if (!lk->active) + die("BUG: fdopen_lock_file() called for unlocked object"); + if (lk->fp) + die("BUG: fdopen_lock_file() called twice for file '%s'", lk->filename.buf); + + lk->fp = fdopen(lk->fd, mode); + return lk->fp; +} + char *get_locked_file_path(struct lock_file *lk) { if (!lk->active) @@ -226,17 +247,32 @@ char *get_locked_file_path(struct lock_file *lk) int close_lock_file(struct lock_file *lk) { int fd = lk->fd; + FILE *fp = lk->fp; + int err; if (fd < 0) return 0; lk->fd = -1; - if (close(fd)) { + if (fp) { + lk->fp = NULL; + + /* + * Note: no short-circuiting here; we want to fclose() + * in any case! + */ + err = ferror(fp) | fclose(fp); + } else { + err = close(fd); + } + + if (err) { int save_errno = errno; rollback_lock_file(lk); errno = save_errno; return -1; } + return 0; } diff --git a/lockfile.h b/lockfile.h index 9059e8958f..dc066d1783 100644 --- a/lockfile.h +++ b/lockfile.h @@ -34,6 +34,8 @@ * - active is set * - filename holds the filename of the lockfile * - fd holds a file descriptor open for writing to the lockfile + * - fp holds a pointer to an open FILE object if and only if + * fdopen_lock_file() has been called on the object * - owner holds the PID of the process that locked the file * * - Locked, lockfile closed (after successful close_lock_file()). @@ -56,6 +58,7 @@ struct lock_file { struct lock_file *volatile next; volatile sig_atomic_t active; volatile int fd; + FILE *volatile fp; volatile pid_t owner; char on_list; struct strbuf filename; @@ -74,6 +77,7 @@ extern void unable_to_lock_message(const char *path, int err, extern NORETURN void unable_to_lock_die(const char *path, int err); 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 FILE *fdopen_lock_file(struct lock_file *, const char *mode); extern char *get_locked_file_path(struct lock_file *); extern int commit_lock_file_to(struct lock_file *, const char *path); extern int commit_lock_file(struct lock_file *); From f70f0565b3a905ba90af3446475afec2e8aa0d2a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 13:14:48 +0200 Subject: [PATCH 2/3] dump_marks(): reimplement using fdopen_lock_file() Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- fast-import.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/fast-import.c b/fast-import.c index deadc33f94..fee7906e51 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1794,20 +1794,18 @@ static void dump_marks_helper(FILE *f, static void dump_marks(void) { static struct lock_file mark_lock; - int mark_fd; FILE *f; if (!export_marks_file) return; - mark_fd = hold_lock_file_for_update(&mark_lock, export_marks_file, 0); - if (mark_fd < 0) { + if (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) { failure |= error("Unable to write marks file %s: %s", export_marks_file, strerror(errno)); return; } - f = fdopen(mark_fd, "w"); + f = fdopen_lock_file(&mark_lock, "w"); if (!f) { int saved_errno = errno; rollback_lock_file(&mark_lock); @@ -1816,22 +1814,7 @@ static void dump_marks(void) return; } - /* - * Since the lock file was fdopen()'ed, it should not be close()'ed. - * Assign -1 to the lock file descriptor so that commit_lock_file() - * won't try to close() it. - */ - mark_lock.fd = -1; - dump_marks_helper(f, 0, marks); - if (ferror(f) || fclose(f)) { - int saved_errno = errno; - rollback_lock_file(&mark_lock); - failure |= error("Unable to write marks file %s: %s", - export_marks_file, strerror(saved_errno)); - return; - } - if (commit_lock_file(&mark_lock)) { failure |= error("Unable to commit marks file %s: %s", export_marks_file, strerror(errno)); From 6e578a31e6662c69107eb4587d1024dd9f38cc3c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 13:14:49 +0200 Subject: [PATCH 3/3] commit_packed_refs(): reimplement using fdopen_lock_file() Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 1d73f1daf5..a77458f2f6 100644 --- a/refs.c +++ b/refs.c @@ -2309,16 +2309,13 @@ int commit_packed_refs(void) if (!packed_ref_cache->lock) die("internal error: packed-refs not locked"); - out = fdopen(packed_ref_cache->lock->fd, "w"); + out = fdopen_lock_file(packed_ref_cache->lock, "w"); if (!out) die_errno("unable to fdopen packed-refs descriptor"); fprintf_or_die(out, "%s", PACKED_REFS_HEADER); do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache), 0, write_packed_entry_fn, out); - if (fclose(out)) - die_errno("write error"); - packed_ref_cache->lock->fd = -1; if (commit_lock_file(packed_ref_cache->lock)) { save_errno = errno;