From e197c21807dacadc8305250baa0b9228819189d4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:05 +0200 Subject: [PATCH 01/38] unable_to_lock_die(): rename function from unable_to_lock_index_die() This function is used for other things besides the index, so rename it accordingly. Suggested-by: Jeff King Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/update-index.c | 2 +- cache.h | 2 +- lockfile.c | 6 +++--- refs.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index e8c7fd4d4..6c9598891 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -942,7 +942,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix) if (newfd < 0) { if (refresh_args.flags & REFRESH_QUIET) exit(128); - unable_to_lock_index_die(get_index_file(), lock_error); + unable_to_lock_die(get_index_file(), lock_error); } if (write_locked_index(&the_index, lock_file, COMMIT_LOCK)) die("Unable to write new index file"); diff --git a/cache.h b/cache.h index 8206039cf..0a76d023e 100644 --- a/cache.h +++ b/cache.h @@ -582,7 +582,7 @@ struct lock_file { extern int unable_to_lock_error(const char *path, int err); extern void unable_to_lock_message(const char *path, int err, struct strbuf *buf); -extern NORETURN void unable_to_lock_index_die(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 int commit_lock_file(struct lock_file *); diff --git a/lockfile.c b/lockfile.c index 2a800cef3..f1ce1544d 100644 --- a/lockfile.c +++ b/lockfile.c @@ -185,7 +185,7 @@ int unable_to_lock_error(const char *path, int err) return -1; } -NORETURN void unable_to_lock_index_die(const char *path, int err) +NORETURN void unable_to_lock_die(const char *path, int err) { struct strbuf buf = STRBUF_INIT; @@ -198,7 +198,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags) { int fd = lock_file(lk, path, flags); if (fd < 0 && (flags & LOCK_DIE_ON_ERROR)) - unable_to_lock_index_die(path, errno); + unable_to_lock_die(path, errno); return fd; } @@ -209,7 +209,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) fd = lock_file(lk, path, flags); if (fd < 0) { if (flags & LOCK_DIE_ON_ERROR) - unable_to_lock_index_die(path, errno); + unable_to_lock_die(path, errno); return fd; } diff --git a/refs.c b/refs.c index ffd45e929..0e324770c 100644 --- a/refs.c +++ b/refs.c @@ -2225,7 +2225,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else - unable_to_lock_index_die(ref_file, errno); + unable_to_lock_die(ref_file, errno); } return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; From a5e48669a2bb45e3a2a6daf30635270b90056085 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:06 +0200 Subject: [PATCH 02/38] api-lockfile: revise and expand the documentation Document a couple more functions and the flags argument as used by hold_lock_file_for_update() and hold_lock_file_for_append(). Reorganize the document to make it more accessible. Helped-by: Jonathan Nieder Helped-by: Junio Hamano Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 221 +++++++++++++++++------ 1 file changed, 167 insertions(+), 54 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index dd894043a..99830f3bf 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -3,20 +3,125 @@ lockfile API The lockfile API serves two purposes: -* Mutual exclusion. When we write out a new index file, first - we create a new file `$GIT_DIR/index.lock`, write the new - contents into it, and rename it to the final destination - `$GIT_DIR/index`. We try to create the `$GIT_DIR/index.lock` - file with O_EXCL so that we can notice and fail when somebody - else is already trying to update the index file. - -* Automatic cruft removal. After we create the "lock" file, we - may decide to `die()`, and we would want to make sure that we - remove the file that has not been committed to its final - destination. This is done by remembering the lockfiles we - created in a linked list and cleaning them up from an - `atexit(3)` handler. Outstanding lockfiles are also removed - when the program dies on a signal. +* Mutual exclusion and atomic file updates. When we want to change a + file, we create a lockfile `.lock`, write the new file + contents into it, and then rename the lockfile to its final + destination ``. We create the `.lock` file with + `O_CREAT|O_EXCL` so that we can notice and fail if somebody else has + already locked the file, then atomically rename the lockfile to its + final destination to commit the changes and unlock the file. + +* Automatic cruft removal. If the program exits after we lock a file + but before the changes have been committed, we want to make sure + that we remove the lockfile. This is done by remembering the + lockfiles we have created in a linked list and setting up an + `atexit(3)` handler and a signal handler that clean up the + lockfiles. This mechanism ensures that outstanding lockfiles are + cleaned up if the program exits (including when `die()` is called) + or if the program dies on a signal. + +Please note that lockfiles only block other writers. Readers do not +block, but they are guaranteed to see either the old contents of the +file or the new contents of the file (assuming that the filesystem +implements `rename(2)` atomically). + + +Calling sequence +---------------- + +The caller: + +* Allocates a `struct lock_file` either as a static variable or on the + heap, initialized to zeros. Once you use the structure to call the + `hold_lock_file_*` family of functions, it belongs to the lockfile + subsystem and its storage must remain valid throughout the life of + the program (i.e. you cannot use an on-stack variable to hold this + structure). + +* Attempts to create a lockfile by passing that variable and the path + 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`). + +When finished writing, the caller can: + +* Close the file descriptor and rename the lockfile to its final + destination by calling `commit_lock_file`. + +* Close the file descriptor and remove the lockfile by calling + `rollback_lock_file`. + +* Close the file descriptor without removing or renaming the lockfile + by calling `close_lock_file`, and later call `commit_lock_file`, + `rollback_lock_file`, or `reopen_lock_file`. + +Even after the lockfile is committed or rolled back, the `lock_file` +object must not be freed or altered by the caller. However, it may be +reused; just pass it to another call of `hold_lock_file_for_update` or +`hold_lock_file_for_append`. + +If the program exits before you have called one of `commit_lock_file`, +`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler +will close and remove the lockfile, rolling back 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 later call to +`commit_lock_file` or `rollback_lock_file` or program exit would +result in duplicate calls to `close(2)`. Worse yet, if you `close(2)` +and then later open another file descriptor for a completely different +purpose, then a call to `commit_lock_file` or `rollback_lock_file` +might close that unrelated file descriptor. + + +Error handling +-------------- + +The `hold_lock_file_*` functions return a file descriptor on success +or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On +errors, `errno` describes the reason for failure. Errors can be +reported by passing `errno` to one of the following helper functions: + +unable_to_lock_message:: + + Append an appropriate error message to a `strbuf`. + +unable_to_lock_error:: + + Emit an appropriate error message using `error()`. + +unable_to_lock_die:: + + Emit an appropriate error message and `die()`. + + +Flags +----- + +The following flags can be passed to `hold_lock_file_for_update` or +`hold_lock_file_for_append`: + +LOCK_NODEREF:: + + Usually symbolic links in the destination path are resolved + and the lockfile is created by adding ".lock" to the resolved + path. If `LOCK_NODEREF` is set, then the lockfile is created + by adding ".lock" to the path argument itself. This option is + used, for example, when locking a symbolic reference, which + for backwards-compatibility reasons can be a symbolic link + containing the name of the referred-to-reference. + +LOCK_DIE_ON_ERROR:: + + If a lock is already taken for the file, `die()` with an error + message. If this option is not specified, trying to lock a + file that is already locked returns -1 to the caller. The functions @@ -24,51 +129,59 @@ The functions hold_lock_file_for_update:: - Take a pointer to `struct lock_file`, the filename of - the final destination (e.g. `$GIT_DIR/index`) and a flag - `die_on_error`. Attempt to create a lockfile for the - destination and return the file descriptor for writing - to the file. If `die_on_error` flag is true, it dies if - a lock is already taken for the file; otherwise it - returns a negative integer to the caller on failure. + Take a pointer to `struct lock_file`, the path of the file to + be locked (e.g. `$GIT_DIR/index`) and a flags argument (see + above). Attempt to create a lockfile for the destination and + return the file descriptor for writing to the file. + +hold_lock_file_for_append:: + + Like `hold_lock_file_for_update`, but before returning copy + the existing contents of the file (if any) to the lockfile and + position its write pointer at the end of the file. commit_lock_file:: - Take a pointer to the `struct lock_file` initialized - with an earlier call to `hold_lock_file_for_update()`, - close the file descriptor and rename the lockfile to its - final destination. Returns 0 upon success, a negative - value on failure to close(2) or rename(2). + 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`, close the file descriptor and + rename the lockfile to its final destination. Return 0 upon + success or a negative value on failure to `close(2)` or + `rename(2)`. rollback_lock_file:: - Take a pointer to the `struct lock_file` initialized - with an earlier call to `hold_lock_file_for_update()`, - close the file descriptor and remove the lockfile. + 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`, close the file descriptor and + remove the lockfile. close_lock_file:: - Take a pointer to the `struct lock_file` initialized - with an earlier call to `hold_lock_file_for_update()`, - and close the file descriptor. Returns 0 upon success, - a negative value on failure to close(2). - -Because the structure is used in an `atexit(3)` handler, its -storage has to stay throughout the life of the program. It -cannot be an auto variable allocated on the stack. - -Call `commit_lock_file()` or `rollback_lock_file()` when you are -done writing to the file descriptor. If you do not call either -and simply `exit(3)` from the program, an `atexit(3)` handler -will close and remove the lockfile. - -If you need to close the file descriptor you obtained from -`hold_lock_file_for_update` function yourself, do so by calling -`close_lock_file()`. You should never call `close(2)` yourself! -Otherwise the `struct -lock_file` structure still remembers that the file descriptor -needs to be closed, and a later call to `commit_lock_file()` or -`rollback_lock_file()` will result in duplicate calls to -`close(2)`. Worse yet, if you `close(2)`, open another file -descriptor for completely different purpose, and then call -`commit_lock_file()` or `rollback_lock_file()`, they may close -that unrelated file descriptor. + + 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 or a negative value on failure to + close(2). Usually `commit_lock_file` or `rollback_lock_file` + should be called after `close_lock_file`. + +reopen_lock_file:: + + Re-open a lockfile that has been closed (using + `close_lock_file`) but not yet committed or rolled back. This + can be used to implement a sequence of operations like the + following: + + * Lock file. + + * Write new contents to lockfile, then `close_lock_file` to + cause the contents to be written to disk. + + * Pass the name of the lockfile to another program to allow it + (and nobody else) to inspect the contents you wrote, while + still holding the lock yourself. + + * `reopen_lock_file` to reopen the lockfile. Make further + updates to the contents. + + * `commit_lock_file` to make the final version permanent. From 419f0c0f681b76d720699977abe03d29e22db554 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:07 +0200 Subject: [PATCH 03/38] close_lock_file(): exit (successfully) if file is already closed Suggested-by: Jonathan Nieder Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 6 +++++- read-cache.c | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lockfile.c b/lockfile.c index f1ce1544d..d02c3bf90 100644 --- a/lockfile.c +++ b/lockfile.c @@ -233,6 +233,10 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) int close_lock_file(struct lock_file *lk) { int fd = lk->fd; + + if (fd < 0) + return 0; + lk->fd = -1; return close(fd); } @@ -251,7 +255,7 @@ int commit_lock_file(struct lock_file *lk) { char result_file[PATH_MAX]; size_t i; - if (lk->fd >= 0 && close_lock_file(lk)) + if (close_lock_file(lk)) return -1; strcpy(result_file, lk->filename); i = strlen(result_file) - 5; /* .lock */ diff --git a/read-cache.c b/read-cache.c index 2fc1182f2..5ffb1d7bb 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2042,7 +2042,7 @@ void set_alternate_index_output(const char *name) static int commit_locked_index(struct lock_file *lk) { if (alternate_index_output) { - if (lk->fd >= 0 && close_lock_file(lk)) + if (close_lock_file(lk)) return -1; if (rename(lk->filename, alternate_index_output)) return -1; From 5527d5349b42aeb2ea36edfd2d55016f22fefc08 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:08 +0200 Subject: [PATCH 04/38] rollback_lock_file(): do not clear filename redundantly It is only necessary to clear the lock_file's filename field if it was not already clear. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index d02c3bf90..5330d6ae5 100644 --- a/lockfile.c +++ b/lockfile.c @@ -280,6 +280,6 @@ void rollback_lock_file(struct lock_file *lk) if (lk->fd >= 0) close(lk->fd); unlink_or_warn(lk->filename); + lk->filename[0] = 0; } - lk->filename[0] = 0; } From 9085f8e279146a31ea8bbd102b6c97f5cb22dcdc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:09 +0200 Subject: [PATCH 05/38] rollback_lock_file(): exit early if lock is not active Eliminate a layer of nesting. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lockfile.c b/lockfile.c index 5330d6ae5..e55149a73 100644 --- a/lockfile.c +++ b/lockfile.c @@ -276,10 +276,11 @@ int hold_locked_index(struct lock_file *lk, int die_on_error) void rollback_lock_file(struct lock_file *lk) { - if (lk->filename[0]) { - if (lk->fd >= 0) - close(lk->fd); - unlink_or_warn(lk->filename); - lk->filename[0] = 0; - } + if (!lk->filename[0]) + return; + + if (lk->fd >= 0) + close(lk->fd); + unlink_or_warn(lk->filename); + lk->filename[0] = 0; } From 26f5d3b65fa1b40be570a67f1aaca0e2f085d568 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:10 +0200 Subject: [PATCH 06/38] rollback_lock_file(): set fd to -1 When rolling back the lockfile, call close_lock_file() so that the lock_file's fd field gets set back to -1. This keeps the lock_file object in a valid state, which is important because these objects are allowed to be reused. It also makes it unnecessary to check whether the file has already been closed, because close_lock_file() takes care of that. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lockfile.c b/lockfile.c index e55149a73..3df1e8306 100644 --- a/lockfile.c +++ b/lockfile.c @@ -279,8 +279,7 @@ void rollback_lock_file(struct lock_file *lk) if (!lk->filename[0]) return; - if (lk->fd >= 0) - close(lk->fd); + close_lock_file(lk); unlink_or_warn(lk->filename); lk->filename[0] = 0; } From 41dd4ffaf99532d8344c90a5b1a060ac1f73b232 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:11 +0200 Subject: [PATCH 07/38] lockfile: unlock file if lockfile permissions cannot be adjusted If the call to adjust_shared_perm() fails, lock_file returns -1, which to the caller looks like any other failure to lock the file. So in this case, roll back the lockfile before returning so that the lock file is deleted immediately and the lockfile object is left in a predictable state (namely, unlocked). Previously, the lockfile was retained until process cleanup in this situation. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lockfile.c b/lockfile.c index 3df1e8306..d74de8d32 100644 --- a/lockfile.c +++ b/lockfile.c @@ -153,6 +153,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) int save_errno = errno; error("cannot fix permission bits on %s", lk->filename); + rollback_lock_file(lk); errno = save_errno; return -1; } From ebb8e380e98e83f32c1cc04200d3749ab4c0b90a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:12 +0200 Subject: [PATCH 08/38] hold_lock_file_for_append(): release lock on errors If there is an error copying the old contents to the lockfile, roll back the lockfile before exiting so that the lockfile is not held until process cleanup. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lockfile.c b/lockfile.c index d74de8d32..f4ce79bc2 100644 --- a/lockfile.c +++ b/lockfile.c @@ -219,13 +219,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) if (errno != ENOENT) { if (flags & LOCK_DIE_ON_ERROR) die("cannot open '%s' for copying", path); - close(fd); + rollback_lock_file(lk); return error("cannot open '%s' for copying", path); } } else if (copy_fd(orig_fd, fd)) { if (flags & LOCK_DIE_ON_ERROR) exit(128); - close(fd); + rollback_lock_file(lk); return -1; } return fd; From 04e57d4d32541bc5dba553a31f09aa2ee456bdad Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:13 +0200 Subject: [PATCH 09/38] lock_file(): always initialize and register lock_file object The purpose of this change is to make the state diagram for lock_file objects simpler and deterministic. If locking fails, lock_file() sometimes leaves the lock_file object partly initialized, but sometimes not. It sometimes registers the object in lock_file_list, but sometimes not. This makes the state diagram for lock_file objects effectively indeterministic and hard to reason about. A future patch will also change the filename field into a strbuf, which needs more involved initialization, so it will become even more important that the state of a lock_file object is well-defined after a failed attempt to lock. The ambiguity doesn't currently have any ill effects, because lock_file objects cannot be removed from the lock_file_list anyway. But to make it easier to document and reason about the code, make this behavior consistent: *always* initialize the lock_file object and *always* register it in lock_file_list the first time it is used, regardless of whether an error occurs. While we're at it, make sure that all of the lock_file fields are initialized to values appropriate for an unlocked object; the caller is only responsible for making sure that on_list is set to zero before the first time it is used. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/lockfile.c b/lockfile.c index f4ce79bc2..81143e55a 100644 --- a/lockfile.c +++ b/lockfile.c @@ -129,6 +129,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) */ static const size_t max_path_len = sizeof(lk->filename) - 5; + if (!lock_file_list) { + /* One-time initialization */ + sigchain_push_common(remove_lock_file_on_signal); + atexit(remove_lock_file); + } + + if (!lk->on_list) { + /* Initialize *lk and add it to lock_file_list: */ + lk->fd = -1; + lk->owner = 0; + lk->filename[0] = 0; + lk->next = lock_file_list; + lock_file_list = lk; + lk->on_list = 1; + } + if (strlen(path) >= max_path_len) { errno = ENAMETOOLONG; return -1; @@ -139,16 +155,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcat(lk->filename, ".lock"); lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 <= lk->fd) { - if (!lock_file_list) { - sigchain_push_common(remove_lock_file_on_signal); - atexit(remove_lock_file); - } lk->owner = getpid(); - if (!lk->on_list) { - lk->next = lock_file_list; - lock_file_list = lk; - lk->on_list = 1; - } if (adjust_shared_perm(lk->filename)) { int save_errno = errno; error("cannot fix permission bits on %s", From 0a06f148373285822baf5f3e83696732a556a2d1 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:14 +0200 Subject: [PATCH 10/38] lockfile.c: document the various states of lock_file objects Document the valid states of lock_file objects, how they get into each state, and how the state is encoded in the object's fields. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Signed-off-by: Junio C Hamano --- lockfile.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/lockfile.c b/lockfile.c index 81143e55a..2680dc949 100644 --- a/lockfile.c +++ b/lockfile.c @@ -4,6 +4,48 @@ #include "cache.h" #include "sigchain.h" +/* + * File write-locks as used by Git. + * + * For an overview of how to use the lockfile API, please see + * + * Documentation/technical/api-lockfile.txt + * + * This module keeps track of all locked files in lock_file_list for + * use at cleanup. This list and the lock_file objects that comprise + * it must be kept in self-consistent states at all time, because the + * program can be interrupted any time by a signal, in which case the + * signal handler will walk through the list attempting to clean up + * any open lock files. + * + * A lockfile is owned by the process that created it. The lock_file + * object has an "owner" field that records its owner. This field is + * used to prevent a forked process from closing a lockfile created by + * its parent. + * + * A lock_file object can be in several states: + * + * - Uninitialized. In this state the object's on_list field must be + * zero but the rest of its contents need not be initialized. As + * soon as the object is used in any way, it is irrevocably + * registered in the lock_file_list, and on_list is set. + * + * - Locked, lockfile open (after hold_lock_file_for_update(), + * hold_lock_file_for_append(), or reopen_lock_file()). In this + * state, the lockfile exists, filename holds the filename of the + * lockfile, fd holds a file descriptor open for writing to the + * lockfile, and owner holds the PID of the process that locked the + * file. + * + * - Locked, lockfile closed (after close_lock_file()). Same as the + * previous state, except that the lockfile is closed and fd is -1. + * + * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a + * failed attempt to lock). In this state, filename[0] == '\0' and + * fd is -1. The object is left registered in the lock_file_list, + * and on_list is set. + */ + static struct lock_file *lock_file_list; static void remove_lock_file(void) From 7108ad232fc7a4c889e82b40c52125adc9796ff5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:15 +0200 Subject: [PATCH 11/38] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN There are a few places that use these values, so define constants for them. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ lockfile.c | 11 ++++++----- refs.c | 7 ++++--- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cache.h b/cache.h index 0a76d023e..24891a81d 100644 --- a/cache.h +++ b/cache.h @@ -570,6 +570,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs update" */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX ".lock" +#define LOCK_SUFFIX_LEN 5 + struct lock_file { struct lock_file *next; int fd; diff --git a/lockfile.c b/lockfile.c index 2680dc949..23847fc9f 100644 --- a/lockfile.c +++ b/lockfile.c @@ -166,10 +166,11 @@ static char *resolve_symlink(char *p, size_t s) static int lock_file(struct lock_file *lk, const char *path, int flags) { /* - * subtract 5 from size to make sure there's room for adding - * ".lock" for the lock file name + * subtract LOCK_SUFFIX_LEN from size to make sure there's + * room for adding ".lock" for the lock file name: */ - static const size_t max_path_len = sizeof(lk->filename) - 5; + static const size_t max_path_len = sizeof(lk->filename) - + LOCK_SUFFIX_LEN; if (!lock_file_list) { /* One-time initialization */ @@ -194,7 +195,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) strcpy(lk->filename, path); if (!(flags & LOCK_NODEREF)) resolve_symlink(lk->filename, max_path_len); - strcat(lk->filename, ".lock"); + strcat(lk->filename, LOCK_SUFFIX); lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666); if (0 <= lk->fd) { lk->owner = getpid(); @@ -308,7 +309,7 @@ int commit_lock_file(struct lock_file *lk) if (close_lock_file(lk)) return -1; strcpy(result_file, lk->filename); - i = strlen(result_file) - 5; /* .lock */ + i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ result_file[i] = 0; if (rename(lk->filename, result_file)) return -1; diff --git a/refs.c b/refs.c index 0e324770c..73d6baedb 100644 --- a/refs.c +++ b/refs.c @@ -79,7 +79,8 @@ static int check_refname_component(const char *refname, int flags) if (refname[1] == '\0') return -1; /* Component equals ".". */ } - if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5)) + if (cp - refname >= LOCK_SUFFIX_LEN && + !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN)) return -1; /* Refname ends with ".lock". */ return cp - refname; } @@ -2602,11 +2603,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) { if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) { /* loose */ - int err, i = strlen(lock->lk->filename) - 5; /* .lock */ + int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN; lock->lk->filename[i] = 0; err = unlink_or_warn(lock->lk->filename); - lock->lk->filename[i] = '.'; + lock->lk->filename[i] = LOCK_SUFFIX[0]; if (err && errno != ENOENT) return 1; } From 91f1f1918430f1ee6f9923d949e1543c49f63204 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:16 +0200 Subject: [PATCH 12/38] delete_ref_loose(): don't muck around in the lock_file's filename It's bad manners. Especially since there could be a signal during the call to unlink_or_warn(), in which case the signal handler will see the wrong filename and delete the reference file, leaving the lockfile behind. So make our own copy to work with. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 73d6baedb..a4151315e 100644 --- a/refs.c +++ b/refs.c @@ -2602,12 +2602,15 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err) static int delete_ref_loose(struct ref_lock *lock, int flag) { if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) { - /* loose */ - int err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN; - - lock->lk->filename[i] = 0; - err = unlink_or_warn(lock->lk->filename); - lock->lk->filename[i] = LOCK_SUFFIX[0]; + /* + * loose. The loose file name is the same as the + * lockfile name, minus ".lock": + */ + char *loose_filename = xmemdupz( + lock->lk->filename, + strlen(lock->lk->filename) - LOCK_SUFFIX_LEN); + int err = unlink_or_warn(loose_filename); + free(loose_filename); if (err && errno != ENOENT) return 1; } From 35ff08be099b36d0be855040a296f985d25dfd0a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:17 +0200 Subject: [PATCH 13/38] prepare_index(): declare return value to be (const char *) Declare the return value to be const to make it clear that we aren't giving callers permission to write over the string that it points at. (The return value is the filename field of a struct lock_file, which can be used by a signal handler at any time and therefore shouldn't be tampered with.) Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/commit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index b0fe7847d..70f5935f1 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -315,8 +315,8 @@ static void refresh_cache_or_die(int refresh_flags) die_resolve_conflict("commit"); } -static char *prepare_index(int argc, const char **argv, const char *prefix, - const struct commit *current_head, int is_status) +static const char *prepare_index(int argc, const char **argv, const char *prefix, + const struct commit *current_head, int is_status) { struct string_list partial; struct pathspec pathspec; From e31e949b9f9b5e6dcff42e2242cf1a6fb3686b91 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:18 +0200 Subject: [PATCH 14/38] lock_file(): exit early if lockfile cannot be opened This is a bit easier to read than the old version, which nested part of the non-error code in an "if" block. Signed-off-by: Michael Haggerty Reviewed-by: Ronnie Sahlberg Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/lockfile.c b/lockfile.c index 23847fc9f..a8f32e549 100644 --- a/lockfile.c +++ b/lockfile.c @@ -197,19 +197,18 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) resolve_symlink(lk->filename, max_path_len); strcat(lk->filename, LOCK_SUFFIX); lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666); - if (0 <= lk->fd) { - lk->owner = getpid(); - if (adjust_shared_perm(lk->filename)) { - int save_errno = errno; - error("cannot fix permission bits on %s", - lk->filename); - rollback_lock_file(lk); - errno = save_errno; - return -1; - } - } - else + if (lk->fd < 0) { lk->filename[0] = 0; + return -1; + } + lk->owner = getpid(); + if (adjust_shared_perm(lk->filename)) { + int save_errno = errno; + error("cannot fix permission bits on %s", lk->filename); + rollback_lock_file(lk); + errno = save_errno; + return -1; + } return lk->fd; } From a1754bcce98fa57b9374440c2717ec1159ed8ffb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:19 +0200 Subject: [PATCH 15/38] remove_lock_file(): call rollback_lock_file() It does just what we need. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- lockfile.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lockfile.c b/lockfile.c index a8f32e549..f8205f6b0 100644 --- a/lockfile.c +++ b/lockfile.c @@ -53,12 +53,8 @@ static void remove_lock_file(void) pid_t me = getpid(); while (lock_file_list) { - if (lock_file_list->owner == me && - lock_file_list->filename[0]) { - if (lock_file_list->fd >= 0) - close(lock_file_list->fd); - unlink_or_warn(lock_file_list->filename); - } + if (lock_file_list->owner == me) + rollback_lock_file(lock_file_list); lock_file_list = lock_file_list->next; } } From 4f4713df94e2f3d3adbd39b8fce571e2bd69185e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:20 +0200 Subject: [PATCH 16/38] commit_lock_file(): inline temporary variable Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lockfile.c b/lockfile.c index f8205f6b0..e148227fb 100644 --- a/lockfile.c +++ b/lockfile.c @@ -300,12 +300,14 @@ int reopen_lock_file(struct lock_file *lk) int commit_lock_file(struct lock_file *lk) { char result_file[PATH_MAX]; - size_t i; + if (close_lock_file(lk)) return -1; + strcpy(result_file, lk->filename); - i = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */ - result_file[i] = 0; + /* remove ".lock": */ + result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0; + if (rename(lk->filename, result_file)) return -1; lk->filename[0] = 0; From 8a1c7533e2ee468a505656abab364780b15004fc Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:21 +0200 Subject: [PATCH 17/38] commit_lock_file(): die() if called for unlocked lockfile object It was previously a bug to call commit_lock_file() with a lock_file object that was not active (an illegal access would happen within the function). It was presumably never done, but this would be an easy programming error to overlook. So before continuing, do a consistency check that the lock_file object really is locked. Helped-by: Johannes Sixt Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 3 ++- lockfile.c | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 99830f3bf..653861075 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -147,7 +147,8 @@ commit_lock_file:: `hold_lock_file_for_append`, close the file descriptor and rename the lockfile to its final destination. Return 0 upon success or a negative value on failure to `close(2)` or - `rename(2)`. + `rename(2)`. It is a bug to call `commit_lock_file()` for a + `lock_file` object that is not currently locked. rollback_lock_file:: diff --git a/lockfile.c b/lockfile.c index e148227fb..c897dd8a9 100644 --- a/lockfile.c +++ b/lockfile.c @@ -301,6 +301,9 @@ int commit_lock_file(struct lock_file *lk) { char result_file[PATH_MAX]; + if (!lk->filename[0]) + die("BUG: attempt to commit unlocked object"); + if (close_lock_file(lk)) return -1; From 8e86c155d2962f5dff83c9d0d88b836bf040c1fa Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:22 +0200 Subject: [PATCH 18/38] close_lock_file(): if close fails, roll back If closing an open lockfile fails, then we cannot be sure of the contents of the lockfile, so there is nothing sensible to do but delete it. This change also insures that the lock_file object is left in a defined state in this error path (namely, unlocked). The only caller that is ultimately affected by this change is try_merge_strategy() -> write_locked_index(), which can call close_lock_file() via various execution paths. This caller uses a static lock_file object which previously could have been reused after a failed close_lock_file() even though it was still in locked state. This change causes the lock_file object to be unlocked on failure, thus fixing this error-handling path. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 7 +++--- lockfile.c | 28 +++++++++++++++--------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 653861075..d3bf940ad 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -162,9 +162,10 @@ 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 or a negative value on failure to - close(2). Usually `commit_lock_file` or `rollback_lock_file` - should be called after `close_lock_file`. + Return 0 upon success. On failure to `close(2)`, return a + negative value and rollback the lock file. Usually + `commit_lock_file` or `rollback_lock_file` should eventually + be called if `close_lock_file` succeeds. reopen_lock_file:: diff --git a/lockfile.c b/lockfile.c index c897dd8a9..1d18c67ea 100644 --- a/lockfile.c +++ b/lockfile.c @@ -37,13 +37,14 @@ * lockfile, and owner holds the PID of the process that locked the * file. * - * - Locked, lockfile closed (after close_lock_file()). Same as the - * previous state, except that the lockfile is closed and fd is -1. + * - Locked, lockfile closed (after successful close_lock_file()). + * Same as the previous state, except that the lockfile is closed + * and fd is -1. * - * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a - * failed attempt to lock). In this state, filename[0] == '\0' and - * fd is -1. The object is left registered in the lock_file_list, - * and on_list is set. + * - Unlocked (after commit_lock_file(), rollback_lock_file(), a + * failed attempt to lock, or a failed close_lock_file()). In this + * state, filename[0] == '\0' and fd is -1. The object is left + * registered in the lock_file_list, and on_list is set. */ static struct lock_file *lock_file_list; @@ -284,7 +285,13 @@ int close_lock_file(struct lock_file *lk) return 0; lk->fd = -1; - return close(fd); + if (close(fd)) { + int save_errno = errno; + rollback_lock_file(lk); + errno = save_errno; + return -1; + } + return 0; } int reopen_lock_file(struct lock_file *lk) @@ -330,7 +337,8 @@ void rollback_lock_file(struct lock_file *lk) if (!lk->filename[0]) return; - close_lock_file(lk); - unlink_or_warn(lk->filename); - lk->filename[0] = 0; + if (!close_lock_file(lk)) { + unlink_or_warn(lk->filename); + lk->filename[0] = 0; + } } From 1b1648f46bcb638f558e9cbd2d0e89066c89c44e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:23 +0200 Subject: [PATCH 19/38] commit_lock_file(): rollback lock file on failure to rename If rename() fails, call rollback_lock_file() to delete the lock file (in case it is still present) and reset the filename field to the empty string so that the lockfile object is left in a valid state. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index 1d18c67ea..728ce4976 100644 --- a/lockfile.c +++ b/lockfile.c @@ -318,8 +318,13 @@ int commit_lock_file(struct lock_file *lk) /* remove ".lock": */ result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0; - if (rename(lk->filename, result_file)) + if (rename(lk->filename, result_file)) { + int save_errno = errno; + rollback_lock_file(lk); + errno = save_errno; return -1; + } + lk->filename[0] = 0; return 0; } From d75145acf6d17eb9b0f9d7d8856e523038081311 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:24 +0200 Subject: [PATCH 20/38] api-lockfile: document edge cases * Document the behavior of commit_lock_file() when it fails, namely that it rolls back the lock_file object and sets errno appropriately. * Document the behavior of rollback_lock_file() when called for a lock_file object that has already been committed or rolled back, namely that it is a NOOP. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index d3bf940ad..9805da099 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -100,6 +100,10 @@ unable_to_lock_die:: Emit an appropriate error message and `die()`. +Similarly, `commit_lock_file` and `close_lock_file` return 0 on +success. On failure they set `errno` appropriately, do their best to +roll back the lockfile, and return -1. + Flags ----- @@ -144,18 +148,22 @@ commit_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`, close the file descriptor and + `hold_lock_file_for_append`, close the file descriptor, and rename the lockfile to its final destination. Return 0 upon - success or a negative value on failure to `close(2)` or - `rename(2)`. It is a bug to call `commit_lock_file()` for a - `lock_file` object that is not currently locked. + success. On failure, roll back the lock file and return -1, + with `errno` set to the value from the failing call to + `close(2)` or `rename(2)`. It is a bug to call + `commit_lock_file` for a `lock_file` object that is not + currently locked. rollback_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`, close the file descriptor and - remove the lockfile. + remove the lockfile. It is a NOOP to call + `rollback_lock_file()` for a `lock_file` object that has + already been committed or rolled back. close_lock_file:: @@ -163,7 +171,7 @@ close_lock_file:: 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 rollback the lock file. Usually + negative value and roll back the lock file. Usually `commit_lock_file` or `rollback_lock_file` should eventually be called if `close_lock_file` succeeds. From 32c3ec258e8b12ba29ce591b09a300621dba9b3a Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:25 +0200 Subject: [PATCH 21/38] dump_marks(): remove a redundant call to rollback_lock_file() When commit_lock_file() fails, it now always calls rollback_lock_file() internally, so there is no need to call that function here. Signed-off-by: Michael Haggerty Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- fast-import.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fast-import.c b/fast-import.c index 96b0f4236..783c6840b 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1832,10 +1832,8 @@ static void dump_marks(void) } if (commit_lock_file(&mark_lock)) { - int saved_errno = errno; - rollback_lock_file(&mark_lock); failure |= error("Unable to commit marks file %s: %s", - export_marks_file, strerror(saved_errno)); + export_marks_file, strerror(errno)); return; } } From e831855ecc6783bfe4b681017349c623fc2fe8c8 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:26 +0200 Subject: [PATCH 22/38] git_config_set_multivar_in_file(): avoid call to rollback_lock_file() After commit_lock_file() is called, then the lock_file object is necessarily either committed or rolled back. So there is no need to call rollback_lock_file() again in either of these cases. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- config.c | 1 + 1 file changed, 1 insertion(+) diff --git a/config.c b/config.c index a677eb6c5..123ed2996 100644 --- a/config.c +++ b/config.c @@ -2083,6 +2083,7 @@ int git_config_set_multivar_in_file(const char *config_filename, if (commit_lock_file(lock) < 0) { error("could not commit config file %s", config_filename); ret = CONFIG_NO_WRITE; + lock = NULL; goto out_free; } From 707103fdfd0c03511fa547d9b80638d8160f1a88 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:27 +0200 Subject: [PATCH 23/38] lockfile: avoid transitory invalid states Because remove_lock_file() can be called any time by the signal handler, it is important that any lock_file objects that are in the lock_file_list are always in a valid state. And since lock_file objects are often reused (but are never removed from lock_file_list), that means we have to be careful whenever mutating a lock_file object to always keep it in a well-defined state. This was formerly not the case, because part of the state was encoded by setting lk->filename to the empty string vs. a valid filename. It is wrong to assume that this string can be updated atomically; for example, even strcpy(lk->filename, value) is unsafe. But the old code was even more reckless; for example, strcpy(lk->filename, path); if (!(flags & LOCK_NODEREF)) resolve_symlink(lk->filename, max_path_len); strcat(lk->filename, ".lock"); During the call to resolve_symlink(), lk->filename contained the name of the file that was being locked, not the name of the lockfile. If a signal were raised during that interval, then the signal handler would have deleted the valuable file! We could probably continue to use the filename field to encode the state by being careful to write characters 1..N-1 of the filename first, and then overwrite the NUL at filename[0] with the first character of the filename, but that would be awkward and error-prone. So, instead of using the filename field to determine whether the lock_file object is active, add a new field "lock_file::active" for this purpose. Be careful to set this field only when filename really contains the name of a file that should be deleted on cleanup. Helped-by: Johannes Sixt Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- cache.h | 1 + lockfile.c | 37 ++++++++++++++++++++++++++----------- read-cache.c | 1 + 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/cache.h b/cache.h index 24891a81d..8e25fce3d 100644 --- a/cache.h +++ b/cache.h @@ -576,6 +576,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct struct lock_file { struct lock_file *next; + volatile sig_atomic_t active; int fd; pid_t owner; char on_list; diff --git a/lockfile.c b/lockfile.c index 728ce4976..d35ac448f 100644 --- a/lockfile.c +++ b/lockfile.c @@ -23,7 +23,7 @@ * used to prevent a forked process from closing a lockfile created by * its parent. * - * A lock_file object can be in several states: + * The possible states of a lock_file object are as follows: * * - Uninitialized. In this state the object's on_list field must be * zero but the rest of its contents need not be initialized. As @@ -32,19 +32,27 @@ * * - Locked, lockfile open (after hold_lock_file_for_update(), * hold_lock_file_for_append(), or reopen_lock_file()). In this - * state, the lockfile exists, filename holds the filename of the - * lockfile, fd holds a file descriptor open for writing to the - * lockfile, and owner holds the PID of the process that locked the - * file. + * state: + * - the lockfile exists + * - active is set + * - filename holds the filename of the lockfile + * - fd holds a file descriptor open for writing to the lockfile + * - owner holds the PID of the process that locked the file * * - Locked, lockfile closed (after successful close_lock_file()). * Same as the previous state, except that the lockfile is closed * and fd is -1. * * - Unlocked (after commit_lock_file(), rollback_lock_file(), a - * failed attempt to lock, or a failed close_lock_file()). In this - * state, filename[0] == '\0' and fd is -1. The object is left - * registered in the lock_file_list, and on_list is set. + * failed attempt to lock, or a failed close_lock_file()). In this + * state: + * - active is unset + * - filename[0] == '\0' (usually, though there are transitory states + * in which this condition doesn't hold). Client code should *not* + * rely on this fact! + * - fd is -1 + * - the object is left registered in the lock_file_list, and + * on_list is set. */ static struct lock_file *lock_file_list; @@ -175,9 +183,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) atexit(remove_lock_file); } + if (lk->active) + die("BUG: cannot lock_file(\"%s\") using active struct lock_file", + path); if (!lk->on_list) { /* Initialize *lk and add it to lock_file_list: */ lk->fd = -1; + lk->active = 0; lk->owner = 0; lk->filename[0] = 0; lk->next = lock_file_list; @@ -199,6 +211,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) return -1; } lk->owner = getpid(); + lk->active = 1; if (adjust_shared_perm(lk->filename)) { int save_errno = errno; error("cannot fix permission bits on %s", lk->filename); @@ -298,7 +311,7 @@ int reopen_lock_file(struct lock_file *lk) { if (0 <= lk->fd) die(_("BUG: reopen a lockfile that is still open")); - if (!lk->filename[0]) + if (!lk->active) die(_("BUG: reopen a lockfile that has been committed")); lk->fd = open(lk->filename, O_WRONLY); return lk->fd; @@ -308,7 +321,7 @@ int commit_lock_file(struct lock_file *lk) { char result_file[PATH_MAX]; - if (!lk->filename[0]) + if (!lk->active) die("BUG: attempt to commit unlocked object"); if (close_lock_file(lk)) @@ -325,6 +338,7 @@ int commit_lock_file(struct lock_file *lk) return -1; } + lk->active = 0; lk->filename[0] = 0; return 0; } @@ -339,11 +353,12 @@ int hold_locked_index(struct lock_file *lk, int die_on_error) void rollback_lock_file(struct lock_file *lk) { - if (!lk->filename[0]) + if (!lk->active) return; if (!close_lock_file(lk)) { unlink_or_warn(lk->filename); + lk->active = 0; lk->filename[0] = 0; } } diff --git a/read-cache.c b/read-cache.c index 5ffb1d7bb..af69f344a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2046,6 +2046,7 @@ static int commit_locked_index(struct lock_file *lk) return -1; if (rename(lk->filename, alternate_index_output)) return -1; + lk->active = 0; lk->filename[0] = 0; return 0; } else { From 2091c5062c6fd928e1ad3e7c059243e597cb8bbf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:28 +0200 Subject: [PATCH 24/38] struct lock_file: declare some fields volatile The function remove_lock_file_on_signal() is used as a signal handler. It is not realistic to make the signal handler conform strictly to the C standard, which is very restrictive about what a signal handler is allowed to do. But let's increase the likelihood that it will work: The lock_file_list global variable and several fields from struct lock_file are used by the signal handler. Declare those values "volatile" to (1) force the main process to write the values to RAM promptly, and (2) prevent updates to these fields from being reordered in a way that leaves an opportunity for a jump to the signal handler while the object is in an inconsistent state. We don't mark the filename field volatile because that would prevent the use of strcpy(), and it is anyway unlikely that a compiler re-orders a strcpy() call across other expressions. So in practice it should be possible to get away without "volatile" in the "filename" case. Suggested-by: Johannes Sixt Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- cache.h | 6 +++--- lockfile.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index 8e25fce3d..c2ea6f15e 100644 --- a/cache.h +++ b/cache.h @@ -575,10 +575,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct #define LOCK_SUFFIX_LEN 5 struct lock_file { - struct lock_file *next; + struct lock_file *volatile next; volatile sig_atomic_t active; - int fd; - pid_t owner; + volatile int fd; + volatile pid_t owner; char on_list; char filename[PATH_MAX]; }; diff --git a/lockfile.c b/lockfile.c index d35ac448f..89043f5ac 100644 --- a/lockfile.c +++ b/lockfile.c @@ -55,7 +55,7 @@ * on_list is set. */ -static struct lock_file *lock_file_list; +static struct lock_file *volatile lock_file_list; static void remove_lock_file(void) { From 1fef4b5041e0144e476ffcc8c559bf06fa80340c Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:29 +0200 Subject: [PATCH 25/38] try_merge_strategy(): remove redundant lock_file allocation By the time the "if" block is entered, the lock_file instance from the main function block is no longer in use, so re-use that one instead of allocating a second one. Note that the "lock" variable in the "if" block shadowed the "lock" variable at function scope, so the only change needed is to remove the inner definition. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/merge.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/merge.c b/builtin/merge.c index dff043dac..1ec39394a 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -668,7 +668,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) { int clean, x; struct commit *result; - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); struct commit_list *reversed = NULL; struct merge_options o; struct commit_list *j; From daccee387a7f3e4ca332649d5311b032a71892e2 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:30 +0200 Subject: [PATCH 26/38] try_merge_strategy(): use a statically-allocated lock_file object Even the one lockfile object needn't be allocated each time the function is called. Instead, define one statically-allocated lock_file object and reuse it for every call. Suggested-by: Jeff King Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/merge.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 1ec39394a..be07f2748 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -656,14 +656,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, struct commit_list *remoteheads, struct commit *head, const char *head_arg) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); + static struct lock_file lock; - hold_locked_index(lock, 1); + hold_locked_index(&lock, 1); refresh_cache(REFRESH_QUIET); if (active_cache_changed && - write_locked_index(&the_index, lock, COMMIT_LOCK)) + write_locked_index(&the_index, &lock, COMMIT_LOCK)) return error(_("Unable to write index.")); - rollback_lock_file(lock); + rollback_lock_file(&lock); if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) { int clean, x; @@ -695,13 +695,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, for (j = common; j; j = j->next) commit_list_insert(j->item, &reversed); - hold_locked_index(lock, 1); + hold_locked_index(&lock, 1); clean = merge_recursive(&o, head, remoteheads->item, reversed, &result); if (active_cache_changed && - write_locked_index(&the_index, lock, COMMIT_LOCK)) + write_locked_index(&the_index, &lock, COMMIT_LOCK)) die (_("unable to write %s"), get_index_file()); - rollback_lock_file(lock); + rollback_lock_file(&lock); return clean ? 0 : 1; } else { return try_merge_command(strategy, xopts_nr, xopts, From 3e88e8fc085bbfad142d51a07ef918b9b5ca1d72 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:31 +0200 Subject: [PATCH 27/38] commit_lock_file(): use a strbuf to manage temporary space Avoid relying on the filename length restrictions that are currently checked by lock_file(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lockfile.c b/lockfile.c index 89043f5ac..1dd118f55 100644 --- a/lockfile.c +++ b/lockfile.c @@ -319,7 +319,8 @@ int reopen_lock_file(struct lock_file *lk) int commit_lock_file(struct lock_file *lk) { - char result_file[PATH_MAX]; + static struct strbuf result_file = STRBUF_INIT; + int err; if (!lk->active) die("BUG: attempt to commit unlocked object"); @@ -327,11 +328,12 @@ int commit_lock_file(struct lock_file *lk) if (close_lock_file(lk)) return -1; - strcpy(result_file, lk->filename); /* remove ".lock": */ - result_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0; - - if (rename(lk->filename, result_file)) { + strbuf_add(&result_file, lk->filename, + strlen(lk->filename) - LOCK_SUFFIX_LEN); + err = rename(lk->filename, result_file.buf); + strbuf_reset(&result_file); + if (err) { int save_errno = errno; rollback_lock_file(lk); errno = save_errno; From cf6950d3bfe1447ac04867b1f5654a2fc9c5db96 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:32 +0200 Subject: [PATCH 28/38] lockfile: change lock_file::filename into a strbuf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For now, we still make sure to allocate at least PATH_MAX characters for the strbuf because resolve_symlink() doesn't know how to expand the space for its return value. (That will be fixed in a moment.) Another alternative would be to just use a strbuf as scratch space in lock_file() but then store a pointer to the naked string in struct lock_file. But lock_file objects are often reused. By reusing the same strbuf, we can avoid having to reallocate the string most times when a lock_file object is reused. Helped-by: Torsten Bögershausen Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/commit.c | 12 +++++------ builtin/reflog.c | 2 +- cache.h | 2 +- config.c | 14 ++++++------- lockfile.c | 53 ++++++++++++++++++++++-------------------------- read-cache.c | 4 ++-- refs.c | 6 +++--- shallow.c | 6 +++--- 8 files changed, 47 insertions(+), 52 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 70f5935f1..f55e80986 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -341,7 +341,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("unable to create temporary index")); old_index_env = getenv(INDEX_ENVIRONMENT); - setenv(INDEX_ENVIRONMENT, index_lock.filename, 1); + setenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1); if (interactive_add(argc, argv, prefix, patch_interactive) != 0) die(_("interactive add failed")); @@ -352,7 +352,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix unsetenv(INDEX_ENVIRONMENT); discard_cache(); - read_cache_from(index_lock.filename); + read_cache_from(index_lock.filename.buf); if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) die(_("unable to write index file")); @@ -362,7 +362,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix warning(_("Failed to update main cache tree")); commit_style = COMMIT_NORMAL; - return index_lock.filename; + return index_lock.filename.buf; } /* @@ -385,7 +385,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) die(_("unable to write new_index file")); commit_style = COMMIT_NORMAL; - return index_lock.filename; + return index_lock.filename.buf; } /* @@ -472,9 +472,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("unable to write temporary index file")); discard_cache(); - read_cache_from(false_lock.filename); + read_cache_from(false_lock.filename.buf); - return false_lock.filename; + return false_lock.filename.buf; } static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn, diff --git a/builtin/reflog.c b/builtin/reflog.c index e8a8fb13b..7c78b15c1 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, write_str_in_full(lock->lock_fd, "\n") != 1 || close_ref(lock) < 0)) { status |= error("Couldn't write %s", - lock->lk->filename); + lock->lk->filename.buf); unlink(newlog_path); } else if (rename(newlog_path, log_file)) { status |= error("cannot rename %s to %s", diff --git a/cache.h b/cache.h index c2ea6f15e..f81d95fc3 100644 --- a/cache.h +++ b/cache.h @@ -580,7 +580,7 @@ struct lock_file { volatile int fd; volatile pid_t owner; char on_list; - char filename[PATH_MAX]; + struct strbuf filename; }; #define LOCK_DIE_ON_ERROR 1 #define LOCK_NODEREF 2 diff --git a/config.c b/config.c index 123ed2996..21107797d 100644 --- a/config.c +++ b/config.c @@ -2024,9 +2024,9 @@ int git_config_set_multivar_in_file(const char *config_filename, MAP_PRIVATE, in_fd, 0); close(in_fd); - if (chmod(lock->filename, st.st_mode & 07777) < 0) { + if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) { error("chmod on %s failed: %s", - lock->filename, strerror(errno)); + lock->filename.buf, strerror(errno)); ret = CONFIG_NO_WRITE; goto out_free; } @@ -2106,7 +2106,7 @@ int git_config_set_multivar_in_file(const char *config_filename, return ret; write_err_out: - ret = write_error(lock->filename); + ret = write_error(lock->filename.buf); goto out_free; } @@ -2207,9 +2207,9 @@ int git_config_rename_section_in_file(const char *config_filename, fstat(fileno(config_file), &st); - if (chmod(lock->filename, st.st_mode & 07777) < 0) { + if (chmod(lock->filename.buf, st.st_mode & 07777) < 0) { ret = error("chmod on %s failed: %s", - lock->filename, strerror(errno)); + lock->filename.buf, strerror(errno)); goto out; } @@ -2230,7 +2230,7 @@ int git_config_rename_section_in_file(const char *config_filename, } store.baselen = strlen(new_name); if (!store_write_section(out_fd, new_name)) { - ret = write_error(lock->filename); + ret = write_error(lock->filename.buf); goto out; } /* @@ -2256,7 +2256,7 @@ int git_config_rename_section_in_file(const char *config_filename, continue; length = strlen(output); if (write_in_full(out_fd, output, length) != length) { - ret = write_error(lock->filename); + ret = write_error(lock->filename.buf); goto out; } } diff --git a/lockfile.c b/lockfile.c index 1dd118f55..85c8648c5 100644 --- a/lockfile.c +++ b/lockfile.c @@ -47,9 +47,9 @@ * failed attempt to lock, or a failed close_lock_file()). In this * state: * - active is unset - * - filename[0] == '\0' (usually, though there are transitory states - * in which this condition doesn't hold). Client code should *not* - * rely on this fact! + * - filename is empty (usually, though there are transitory + * states in which this condition doesn't hold). Client code should + * *not* rely on the filename being empty in this state. * - fd is -1 * - the object is left registered in the lock_file_list, and * on_list is set. @@ -170,13 +170,6 @@ static char *resolve_symlink(char *p, size_t s) /* Make sure errno contains a meaningful value on error */ static int lock_file(struct lock_file *lk, const char *path, int flags) { - /* - * subtract LOCK_SUFFIX_LEN from size to make sure there's - * room for adding ".lock" for the lock file name: - */ - static const size_t max_path_len = sizeof(lk->filename) - - LOCK_SUFFIX_LEN; - if (!lock_file_list) { /* One-time initialization */ sigchain_push_common(remove_lock_file_on_signal); @@ -191,30 +184,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) lk->fd = -1; lk->active = 0; lk->owner = 0; - lk->filename[0] = 0; + strbuf_init(&lk->filename, PATH_MAX); lk->next = lock_file_list; lock_file_list = lk; lk->on_list = 1; + } else if (lk->filename.len) { + /* This shouldn't happen, but better safe than sorry. */ + die("BUG: lock_file(\"%s\") called with improperly-reset lock_file object", + path); } - if (strlen(path) >= max_path_len) { - errno = ENAMETOOLONG; - return -1; + strbuf_addstr(&lk->filename, path); + if (!(flags & LOCK_NODEREF)) { + resolve_symlink(lk->filename.buf, lk->filename.alloc); + strbuf_setlen(&lk->filename, strlen(lk->filename.buf)); } - strcpy(lk->filename, path); - if (!(flags & LOCK_NODEREF)) - resolve_symlink(lk->filename, max_path_len); - strcat(lk->filename, LOCK_SUFFIX); - lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666); + strbuf_addstr(&lk->filename, LOCK_SUFFIX); + lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666); if (lk->fd < 0) { - lk->filename[0] = 0; + strbuf_reset(&lk->filename); return -1; } lk->owner = getpid(); lk->active = 1; - if (adjust_shared_perm(lk->filename)) { + if (adjust_shared_perm(lk->filename.buf)) { int save_errno = errno; - error("cannot fix permission bits on %s", lk->filename); + error("cannot fix permission bits on %s", lk->filename.buf); rollback_lock_file(lk); errno = save_errno; return -1; @@ -313,7 +308,7 @@ int reopen_lock_file(struct lock_file *lk) die(_("BUG: reopen a lockfile that is still open")); if (!lk->active) die(_("BUG: reopen a lockfile that has been committed")); - lk->fd = open(lk->filename, O_WRONLY); + lk->fd = open(lk->filename.buf, O_WRONLY); return lk->fd; } @@ -329,9 +324,9 @@ int commit_lock_file(struct lock_file *lk) return -1; /* remove ".lock": */ - strbuf_add(&result_file, lk->filename, - strlen(lk->filename) - LOCK_SUFFIX_LEN); - err = rename(lk->filename, result_file.buf); + strbuf_add(&result_file, lk->filename.buf, + lk->filename.len - LOCK_SUFFIX_LEN); + err = rename(lk->filename.buf, result_file.buf); strbuf_reset(&result_file); if (err) { int save_errno = errno; @@ -341,7 +336,7 @@ int commit_lock_file(struct lock_file *lk) } lk->active = 0; - lk->filename[0] = 0; + strbuf_reset(&lk->filename); return 0; } @@ -359,8 +354,8 @@ void rollback_lock_file(struct lock_file *lk) return; if (!close_lock_file(lk)) { - unlink_or_warn(lk->filename); + unlink_or_warn(lk->filename.buf); lk->active = 0; - lk->filename[0] = 0; + strbuf_reset(&lk->filename); } } diff --git a/read-cache.c b/read-cache.c index af69f344a..91bf876ee 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2044,10 +2044,10 @@ static int commit_locked_index(struct lock_file *lk) if (alternate_index_output) { if (close_lock_file(lk)) return -1; - if (rename(lk->filename, alternate_index_output)) + if (rename(lk->filename.buf, alternate_index_output)) return -1; lk->active = 0; - lk->filename[0] = 0; + strbuf_reset(&lk->filename); return 0; } else { return commit_lock_file(lk); diff --git a/refs.c b/refs.c index a4151315e..598f4eb9e 100644 --- a/refs.c +++ b/refs.c @@ -2607,8 +2607,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) * lockfile name, minus ".lock": */ char *loose_filename = xmemdupz( - lock->lk->filename, - strlen(lock->lk->filename) - LOCK_SUFFIX_LEN); + lock->lk->filename.buf, + lock->lk->filename.len - LOCK_SUFFIX_LEN); int err = unlink_or_warn(loose_filename); free(loose_filename); if (err && errno != ENOENT) @@ -2972,7 +2972,7 @@ int write_ref_sha1(struct ref_lock *lock, write_in_full(lock->lock_fd, &term, 1) != 1 || close_ref(lock) < 0) { int save_errno = errno; - error("Couldn't write %s", lock->lk->filename); + error("Couldn't write %s", lock->lk->filename.buf); unlock_ref(lock); errno = save_errno; return -1; diff --git a/shallow.c b/shallow.c index 57f4afa6b..4919baf77 100644 --- a/shallow.c +++ b/shallow.c @@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock, if (write_shallow_commits(&sb, 0, extra)) { if (write_in_full(fd, sb.buf, sb.len) != sb.len) die_errno("failed to write to %s", - shallow_lock->filename); - *alternate_shallow_file = shallow_lock->filename; + shallow_lock->filename.buf); + *alternate_shallow_file = shallow_lock->filename.buf; } else /* * is_repository_shallow() sees empty string as "no @@ -316,7 +316,7 @@ void prune_shallow(int show_only) if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) { if (write_in_full(fd, sb.buf, sb.len) != sb.len) die_errno("failed to write to %s", - shallow_lock.filename); + shallow_lock.filename.buf); commit_lock_file(&shallow_lock); } else { unlink(git_path("shallow")); From 5025d8450a4c8bf9d22a202433072bee780b9b72 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:33 +0200 Subject: [PATCH 29/38] resolve_symlink(): use a strbuf for internal scratch space Aside from shortening and simplifying the code, this removes another place where the path name length is arbitrarily limited. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/lockfile.c b/lockfile.c index 85c8648c5..cc9b9cbaf 100644 --- a/lockfile.c +++ b/lockfile.c @@ -126,44 +126,35 @@ static char *last_path_elm(char *p) static char *resolve_symlink(char *p, size_t s) { int depth = MAXDEPTH; + static struct strbuf link = STRBUF_INIT; while (depth--) { - char link[PATH_MAX]; - int link_len = readlink(p, link, sizeof(link)); - if (link_len < 0) { - /* not a symlink anymore */ - return p; - } - else if (link_len < sizeof(link)) - /* readlink() never null-terminates */ - link[link_len] = '\0'; - else { - warning("%s: symlink too long", p); - return p; - } + if (strbuf_readlink(&link, p, strlen(p)) < 0) + break; - if (is_absolute_path(link)) { + if (is_absolute_path(link.buf)) { /* absolute path simply replaces p */ - if (link_len < s) - strcpy(p, link); + if (link.len < s) + strcpy(p, link.buf); else { warning("%s: symlink too long", p); - return p; + break; } } else { /* - * link is a relative path, so I must replace the + * link is a relative path, so replace the * last element of p with it. */ char *r = (char *)last_path_elm(p); - if (r - p + link_len < s) - strcpy(r, link); + if (r - p + link.len < s) + strcpy(r, link.buf); else { warning("%s: symlink too long", p); - return p; + break; } } } + strbuf_reset(&link); return p; } From 6cad805332e3c3a60993f062afd8d896920689ab Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:34 +0200 Subject: [PATCH 30/38] resolve_symlink(): take a strbuf parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change resolve_symlink() to take a strbuf rather than a string as parameter. This simplifies the code and removes an arbitrary pathname length restriction. It also means that lock_file's filename field no longer needs to be initialized to a large size. Helped-by: Torsten Bögershausen Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 57 +++++++++++++++++++++--------------------------------- 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/lockfile.c b/lockfile.c index cc9b9cbaf..5f5bcfffb 100644 --- a/lockfile.c +++ b/lockfile.c @@ -109,58 +109,47 @@ static char *last_path_elm(char *p) #define MAXDEPTH 5 /* - * p = path that may be a symlink - * s = full size of p + * path contains a path that might be a symlink. * - * If p is a symlink, attempt to overwrite p with a path to the real - * file or directory (which may or may not exist), following a chain of - * symlinks if necessary. Otherwise, leave p unmodified. + * If path is a symlink, attempt to overwrite it with a path to the + * real file or directory (which may or may not exist), following a + * chain of symlinks if necessary. Otherwise, leave path unmodified. * - * This is a best-effort routine. If an error occurs, p will either be - * left unmodified or will name a different symlink in a symlink chain - * that started with p's initial contents. - * - * Always returns p. + * This is a best-effort routine. If an error occurs, path will + * either be left unmodified or will name a different symlink in a + * symlink chain that started with the original path. */ - -static char *resolve_symlink(char *p, size_t s) +static void resolve_symlink(struct strbuf *path) { int depth = MAXDEPTH; static struct strbuf link = STRBUF_INIT; while (depth--) { - if (strbuf_readlink(&link, p, strlen(p)) < 0) + if (strbuf_readlink(&link, path->buf, path->len) < 0) break; - if (is_absolute_path(link.buf)) { + if (is_absolute_path(link.buf)) /* absolute path simply replaces p */ - if (link.len < s) - strcpy(p, link.buf); - else { - warning("%s: symlink too long", p); - break; - } - } else { + strbuf_reset(path); + else { /* * link is a relative path, so replace the * last element of p with it. */ - char *r = (char *)last_path_elm(p); - if (r - p + link.len < s) - strcpy(r, link.buf); - else { - warning("%s: symlink too long", p); - break; - } + char *r = last_path_elm(path->buf); + strbuf_setlen(path, r - path->buf); } + + strbuf_addbuf(path, &link); } strbuf_reset(&link); - return p; } /* Make sure errno contains a meaningful value on error */ static int lock_file(struct lock_file *lk, const char *path, int flags) { + size_t pathlen = strlen(path); + if (!lock_file_list) { /* One-time initialization */ sigchain_push_common(remove_lock_file_on_signal); @@ -175,7 +164,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) lk->fd = -1; lk->active = 0; lk->owner = 0; - strbuf_init(&lk->filename, PATH_MAX); + strbuf_init(&lk->filename, pathlen + LOCK_SUFFIX_LEN); lk->next = lock_file_list; lock_file_list = lk; lk->on_list = 1; @@ -185,11 +174,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) path); } - strbuf_addstr(&lk->filename, path); - if (!(flags & LOCK_NODEREF)) { - resolve_symlink(lk->filename.buf, lk->filename.alloc); - strbuf_setlen(&lk->filename, strlen(lk->filename.buf)); - } + strbuf_add(&lk->filename, path, pathlen); + if (!(flags & LOCK_NODEREF)) + resolve_symlink(&lk->filename); strbuf_addstr(&lk->filename, LOCK_SUFFIX); lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666); if (lk->fd < 0) { From 0c0d6e8601a1cfb8ebbdadb6a25a9f6fadc91359 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:35 +0200 Subject: [PATCH 31/38] trim_last_path_component(): replace last_path_elm() Rewrite last_path_elm() to take a strbuf parameter and to trim off the last path name element in place rather than returning a pointer to the beginning of the last path name element. This simplifies the function a bit and makes it integrate better with its caller, which is now also strbuf-based. Rename the function accordingly and a bit less tersely. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/lockfile.c b/lockfile.c index 5f5bcfffb..56ad7e8a6 100644 --- a/lockfile.c +++ b/lockfile.c @@ -76,32 +76,28 @@ static void remove_lock_file_on_signal(int signo) } /* - * p = absolute or relative path name + * path = absolute or relative path name * - * Return a pointer into p showing the beginning of the last path name - * element. If p is empty or the root directory ("/"), just return p. + * Remove the last path name element from path (leaving the preceding + * "/", if any). If path is empty or the root directory ("/"), set + * path to the empty string. */ -static char *last_path_elm(char *p) +static void trim_last_path_component(struct strbuf *path) { - /* r starts pointing to null at the end of the string */ - char *r = strchr(p, '\0'); - - if (r == p) - return p; /* just return empty string */ - - r--; /* back up to last non-null character */ + int i = path->len; /* back up past trailing slashes, if any */ - while (r > p && *r == '/') - r--; + while (i && path->buf[i - 1] == '/') + i--; /* - * then go backwards until I hit a slash, or the beginning of - * the string + * then go backwards until a slash, or the beginning of the + * string */ - while (r > p && *(r-1) != '/') - r--; - return r; + while (i && path->buf[i - 1] != '/') + i--; + + strbuf_setlen(path, i); } @@ -131,14 +127,12 @@ static void resolve_symlink(struct strbuf *path) if (is_absolute_path(link.buf)) /* absolute path simply replaces p */ strbuf_reset(path); - else { + else /* * link is a relative path, so replace the * last element of p with it. */ - char *r = last_path_elm(path->buf); - strbuf_setlen(path, r - path->buf); - } + trim_last_path_component(path); strbuf_addbuf(path, &link); } From 751bacedaa507b7b6d10b2c1f48e019a01a8fa6e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:36 +0200 Subject: [PATCH 32/38] commit_lock_file_to(): refactor a helper out of commit_lock_file() commit_locked_index(), when writing to an alternate index file, duplicates (poorly) the code in commit_lock_file(). And anyway, it shouldn't have to know so much about the internal workings of lockfile objects. So extract a new function commit_lock_file_to() that does the work common to the two functions, and call it from both commit_lock_file() and commit_locked_index(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 34 +++++++++++--------- cache.h | 1 + lockfile.c | 40 +++++++++++++++--------- read-cache.c | 13 ++------ 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 9805da099..aa7d82290 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -49,14 +49,14 @@ The caller: When finished writing, the caller can: * Close the file descriptor and rename the lockfile to its final - destination by calling `commit_lock_file`. + destination by calling `commit_lock_file` or `commit_lock_file_to`. * Close the file descriptor and remove the lockfile by calling `rollback_lock_file`. * Close the file descriptor without removing or renaming the lockfile by calling `close_lock_file`, and later call `commit_lock_file`, - `rollback_lock_file`, or `reopen_lock_file`. + `commit_lock_file_to`, `rollback_lock_file`, or `reopen_lock_file`. Even after the lockfile is committed or rolled back, the `lock_file` object must not be freed or altered by the caller. However, it may be @@ -64,20 +64,19 @@ reused; just pass it to another call of `hold_lock_file_for_update` or `hold_lock_file_for_append`. If the program exits before you have called one of `commit_lock_file`, -`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler -will close and remove the lockfile, rolling back any uncommitted -changes. +`commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an +`atexit(3)` handler will close and remove the lockfile, rolling back +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 later call to -`commit_lock_file` or `rollback_lock_file` or program exit would +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)` and then later open another file descriptor for a completely different -purpose, then a call to `commit_lock_file` or `rollback_lock_file` -might close that unrelated file descriptor. +purpose, then a commit or rollback might close that unrelated file +descriptor. Error handling @@ -100,9 +99,9 @@ unable_to_lock_die:: Emit an appropriate error message and `die()`. -Similarly, `commit_lock_file` and `close_lock_file` return 0 on -success. On failure they set `errno` appropriately, do their best to -roll back the lockfile, and return -1. +Similarly, `commit_lock_file`, `commit_lock_file_to`, and +`close_lock_file` return 0 on success. On failure they set `errno` +appropriately, do their best to roll back the lockfile, and return -1. Flags @@ -156,6 +155,12 @@ commit_lock_file:: `commit_lock_file` for a `lock_file` object that is not currently locked. +commit_lock_file_to:: + + Like `commit_lock_file()`, except that it takes an explicit + `path` argument to which the lockfile should be renamed. The + `path` must be on the same filesystem as the lock file. + rollback_lock_file:: Take a pointer to the `struct lock_file` initialized with an @@ -172,8 +177,9 @@ close_lock_file:: `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` or `rollback_lock_file` should eventually - be called if `close_lock_file` succeeds. + `commit_lock_file`, `commit_lock_file_to`, or + `rollback_lock_file` should eventually be called if + `close_lock_file` succeeds. reopen_lock_file:: diff --git a/cache.h b/cache.h index f81d95fc3..414e93ca6 100644 --- a/cache.h +++ b/cache.h @@ -590,6 +590,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 int commit_lock_file_to(struct lock_file *, const char *path); extern int commit_lock_file(struct lock_file *); extern int reopen_lock_file(struct lock_file *); extern void update_index_if_able(struct index_state *, struct lock_file *); diff --git a/lockfile.c b/lockfile.c index 56ad7e8a6..cf7f4d047 100644 --- a/lockfile.c +++ b/lockfile.c @@ -43,9 +43,9 @@ * Same as the previous state, except that the lockfile is closed * and fd is -1. * - * - Unlocked (after commit_lock_file(), rollback_lock_file(), a - * failed attempt to lock, or a failed close_lock_file()). In this - * state: + * - Unlocked (after commit_lock_file(), commit_lock_file_to(), + * rollback_lock_file(), a failed attempt to lock, or a failed + * close_lock_file()). In this state: * - active is unset * - filename is empty (usually, though there are transitory * states in which this condition doesn't hold). Client code should @@ -284,23 +284,15 @@ int reopen_lock_file(struct lock_file *lk) return lk->fd; } -int commit_lock_file(struct lock_file *lk) +int commit_lock_file_to(struct lock_file *lk, const char *path) { - static struct strbuf result_file = STRBUF_INIT; - int err; - if (!lk->active) - die("BUG: attempt to commit unlocked object"); + die("BUG: attempt to commit unlocked object to \"%s\"", path); if (close_lock_file(lk)) return -1; - /* remove ".lock": */ - strbuf_add(&result_file, lk->filename.buf, - lk->filename.len - LOCK_SUFFIX_LEN); - err = rename(lk->filename.buf, result_file.buf); - strbuf_reset(&result_file); - if (err) { + if (rename(lk->filename.buf, path)) { int save_errno = errno; rollback_lock_file(lk); errno = save_errno; @@ -312,6 +304,26 @@ int commit_lock_file(struct lock_file *lk) return 0; } +int commit_lock_file(struct lock_file *lk) +{ + static struct strbuf result_file = STRBUF_INIT; + int err; + + if (!lk->active) + die("BUG: attempt to commit unlocked object"); + + if (lk->filename.len <= LOCK_SUFFIX_LEN || + strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX)) + die("BUG: lockfile filename corrupt"); + + /* remove ".lock": */ + strbuf_add(&result_file, lk->filename.buf, + lk->filename.len - LOCK_SUFFIX_LEN); + err = commit_lock_file_to(lk, result_file.buf); + strbuf_reset(&result_file); + return err; +} + int hold_locked_index(struct lock_file *lk, int die_on_error) { return hold_lock_file_for_update(lk, get_index_file(), diff --git a/read-cache.c b/read-cache.c index 91bf876ee..e887e23fb 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2041,17 +2041,10 @@ void set_alternate_index_output(const char *name) static int commit_locked_index(struct lock_file *lk) { - if (alternate_index_output) { - if (close_lock_file(lk)) - return -1; - if (rename(lk->filename.buf, alternate_index_output)) - return -1; - lk->active = 0; - strbuf_reset(&lk->filename); - return 0; - } else { + if (alternate_index_output) + return commit_lock_file_to(lk, alternate_index_output); + else return commit_lock_file(lk); - } } static int do_write_locked_index(struct index_state *istate, struct lock_file *lock, From 47ba4662bfd755829edd24428cf2e4bc492d70a6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:37 +0200 Subject: [PATCH 33/38] lockfile: rename LOCK_NODEREF to LOCK_NO_DEREF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes it harder to misread the name as LOCK_NODE_REF. Suggested-by: Torsten Bögershausen Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 4 ++-- cache.h | 2 +- lockfile.c | 2 +- refs.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index aa7d82290..a3cb69b96 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -110,11 +110,11 @@ Flags The following flags can be passed to `hold_lock_file_for_update` or `hold_lock_file_for_append`: -LOCK_NODEREF:: +LOCK_NO_DEREF:: Usually symbolic links in the destination path are resolved and the lockfile is created by adding ".lock" to the resolved - path. If `LOCK_NODEREF` is set, then the lockfile is created + path. If `LOCK_NO_DEREF` is set, then the lockfile is created by adding ".lock" to the path argument itself. This option is used, for example, when locking a symbolic reference, which for backwards-compatibility reasons can be a symbolic link diff --git a/cache.h b/cache.h index 414e93ca6..7ea4e8125 100644 --- a/cache.h +++ b/cache.h @@ -583,7 +583,7 @@ struct lock_file { struct strbuf filename; }; #define LOCK_DIE_ON_ERROR 1 -#define LOCK_NODEREF 2 +#define LOCK_NO_DEREF 2 extern int unable_to_lock_error(const char *path, int err); extern void unable_to_lock_message(const char *path, int err, struct strbuf *buf); diff --git a/lockfile.c b/lockfile.c index cf7f4d047..a1cc08a5f 100644 --- a/lockfile.c +++ b/lockfile.c @@ -169,7 +169,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) } strbuf_add(&lk->filename, path, pathlen); - if (!(flags & LOCK_NODEREF)) + if (!(flags & LOCK_NO_DEREF)) resolve_symlink(&lk->filename); strbuf_addstr(&lk->filename, LOCK_SUFFIX); lk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666); diff --git a/refs.c b/refs.c index 598f4eb9e..c10eaff57 100644 --- a/refs.c +++ b/refs.c @@ -2192,7 +2192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, lflags = 0; if (flags & REF_NODEREF) { refname = orig_refname; - lflags |= LOCK_NODEREF; + lflags |= LOCK_NO_DEREF; } lock->ref_name = xstrdup(refname); lock->orig_ref_name = xstrdup(orig_refname); From 316683bd37608e31cc3f5e932c4e5c7dde1b39f0 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:38 +0200 Subject: [PATCH 34/38] lockfile.c: rename static functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * remove_lock_file() -> remove_lock_files() * remove_lock_file_on_signal() -> remove_lock_files_on_signal() Suggested-by: Torsten Bögershausen Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lockfile.c b/lockfile.c index a1cc08a5f..0a8c3c881 100644 --- a/lockfile.c +++ b/lockfile.c @@ -57,7 +57,7 @@ static struct lock_file *volatile lock_file_list; -static void remove_lock_file(void) +static void remove_lock_files(void) { pid_t me = getpid(); @@ -68,9 +68,9 @@ static void remove_lock_file(void) } } -static void remove_lock_file_on_signal(int signo) +static void remove_lock_files_on_signal(int signo) { - remove_lock_file(); + remove_lock_files(); sigchain_pop(signo); raise(signo); } @@ -146,8 +146,8 @@ 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_file_on_signal); - atexit(remove_lock_file); + sigchain_push_common(remove_lock_files_on_signal); + atexit(remove_lock_files); } if (lk->active) From ec38b4e482e96e62762452cab5714e55abdb48c3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:39 +0200 Subject: [PATCH 35/38] get_locked_file_path(): new function Add a function to return the path of the file that is locked by a lock_file object. This reduces the knowledge that callers have to have about the lock_file layout. Suggested-by: Ronnie Sahlberg Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 5 +++++ cache.h | 1 + lockfile.c | 9 +++++++++ refs.c | 4 +--- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index a3cb69b96..d4484d154 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -143,6 +143,11 @@ 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. +get_locked_file_path:: + + Return the path of the file that is locked by the specified + lock_file object. The caller must free the memory. + commit_lock_file:: Take a pointer to the `struct lock_file` initialized with an diff --git a/cache.h b/cache.h index 7ea4e8125..d19e57ff6 100644 --- a/cache.h +++ b/cache.h @@ -590,6 +590,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 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 *); extern int reopen_lock_file(struct lock_file *); diff --git a/lockfile.c b/lockfile.c index 0a8c3c881..c51c6ec69 100644 --- a/lockfile.c +++ b/lockfile.c @@ -257,6 +257,15 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) return fd; } +char *get_locked_file_path(struct lock_file *lk) +{ + if (!lk->active) + die("BUG: get_locked_file_path() called for unlocked object"); + if (lk->filename.len <= LOCK_SUFFIX_LEN) + die("BUG: get_locked_file_path() called for malformed lock object"); + return xmemdupz(lk->filename.buf, lk->filename.len - LOCK_SUFFIX_LEN); +} + int close_lock_file(struct lock_file *lk) { int fd = lk->fd; diff --git a/refs.c b/refs.c index c10eaff57..e40c47edc 100644 --- a/refs.c +++ b/refs.c @@ -2606,9 +2606,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag) * loose. The loose file name is the same as the * lockfile name, minus ".lock": */ - char *loose_filename = xmemdupz( - lock->lk->filename.buf, - lock->lk->filename.len - LOCK_SUFFIX_LEN); + char *loose_filename = get_locked_file_path(lock->lk); int err = unlink_or_warn(loose_filename); free(loose_filename); if (err && errno != ENOENT) From 4d423a3e62c7ab0b04c4bd84995c32daff3b24c3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:40 +0200 Subject: [PATCH 36/38] hold_lock_file_for_append(): restore errno before returning Callers who don't pass LOCK_DIE_ON_ERROR might want to examine errno to see what went wrong, so restore errno before returning. In fact this function only has one caller, add_to_alternates_file(), and it *does* use LOCK_DIE_ON_ERROR, but, you know, think of future generations. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lockfile.c b/lockfile.c index c51c6ec69..b2f5d36f7 100644 --- a/lockfile.c +++ b/lockfile.c @@ -243,15 +243,22 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) orig_fd = open(path, O_RDONLY); if (orig_fd < 0) { if (errno != ENOENT) { + int save_errno = errno; + if (flags & LOCK_DIE_ON_ERROR) die("cannot open '%s' for copying", path); rollback_lock_file(lk); - return error("cannot open '%s' for copying", path); + error("cannot open '%s' for copying", path); + errno = save_errno; + return -1; } } else if (copy_fd(orig_fd, fd)) { + int save_errno = errno; + if (flags & LOCK_DIE_ON_ERROR) exit(128); rollback_lock_file(lk); + errno = save_errno; return -1; } return fd; From 216aab1e3d8eef088dc9785febce24a110e9f835 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:41 +0200 Subject: [PATCH 37/38] hold_locked_index(): move from lockfile.c to read-cache.c lockfile.c contains the general API for locking any file. Code specifically about the index file doesn't belong here. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- lockfile.c | 8 -------- read-cache.c | 8 ++++++++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lockfile.c b/lockfile.c index b2f5d36f7..63f4e94bc 100644 --- a/lockfile.c +++ b/lockfile.c @@ -340,14 +340,6 @@ int commit_lock_file(struct lock_file *lk) return err; } -int hold_locked_index(struct lock_file *lk, int die_on_error) -{ - return hold_lock_file_for_update(lk, get_index_file(), - die_on_error - ? LOCK_DIE_ON_ERROR - : 0); -} - void rollback_lock_file(struct lock_file *lk) { if (!lk->active) diff --git a/read-cache.c b/read-cache.c index e887e23fb..9f137e7f2 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1367,6 +1367,14 @@ static int read_index_extension(struct index_state *istate, return 0; } +int hold_locked_index(struct lock_file *lk, int die_on_error) +{ + return hold_lock_file_for_update(lk, get_index_file(), + die_on_error + ? LOCK_DIE_ON_ERROR + : 0); +} + int read_index(struct index_state *istate) { return read_index_from(istate, get_index_file()); From 697cc8efd944a32ca472337cd6640004c474b788 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:42 +0200 Subject: [PATCH 38/38] lockfile.h: extract new header file for the functions in lockfile.c Move the interface declaration for the functions in lockfile.c from cache.h to a new file, lockfile.h. Add #includes where necessary (and remove some redundant includes of cache.h by files that already include builtin.h). Move the documentation of the lock_file state diagram from lockfile.c to the new header file. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- builtin/add.c | 1 + builtin/apply.c | 1 + builtin/checkout-index.c | 2 +- builtin/checkout.c | 2 +- builtin/clone.c | 1 + builtin/commit.c | 1 + builtin/describe.c | 1 + builtin/diff.c | 1 + builtin/gc.c | 2 +- builtin/merge.c | 1 + builtin/mv.c | 2 +- builtin/read-tree.c | 1 + builtin/receive-pack.c | 1 + builtin/reflog.c | 2 +- builtin/reset.c | 1 + builtin/rm.c | 2 +- builtin/update-index.c | 1 + bundle.c | 1 + cache-tree.c | 1 + cache.h | 27 +------------ config.c | 1 + credential-store.c | 1 + fast-import.c | 1 + fetch-pack.c | 1 + lockfile.c | 52 +------------------------ lockfile.h | 84 ++++++++++++++++++++++++++++++++++++++++ merge-recursive.c | 1 + merge.c | 1 + read-cache.c | 1 + refs.c | 1 + rerere.c | 1 + sequencer.c | 1 + sha1_file.c | 1 + shallow.c | 1 + test-scrap-cache-tree.c | 1 + 35 files changed, 118 insertions(+), 83 deletions(-) create mode 100644 lockfile.h diff --git a/builtin/add.c b/builtin/add.c index 352b85e8d..ae6d3e262 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -5,6 +5,7 @@ */ #include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "dir.h" #include "pathspec.h" #include "exec_cmd.h" diff --git a/builtin/apply.c b/builtin/apply.c index 8714a8872..69efb0e4d 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -7,6 +7,7 @@ * */ #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "quote.h" #include "blob.h" diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 05edd9e1d..383dccf93 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -5,7 +5,7 @@ * */ #include "builtin.h" -#include "cache.h" +#include "lockfile.h" #include "quote.h" #include "cache-tree.h" #include "parse-options.h" diff --git a/builtin/checkout.c b/builtin/checkout.c index 8afdf2b5c..570bb09c4 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1,5 +1,5 @@ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "parse-options.h" #include "refs.h" #include "commit.h" diff --git a/builtin/clone.c b/builtin/clone.c index 3927edfb6..d3bf9532d 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -9,6 +9,7 @@ */ #include "builtin.h" +#include "lockfile.h" #include "parse-options.h" #include "fetch-pack.h" #include "refs.h" diff --git a/builtin/commit.c b/builtin/commit.c index f55e80986..c2300185e 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -6,6 +6,7 @@ */ #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "color.h" #include "dir.h" diff --git a/builtin/describe.c b/builtin/describe.c index ee6a3b998..9103193b4 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "commit.h" #include "tag.h" #include "refs.h" diff --git a/builtin/diff.c b/builtin/diff.c index 0f247d240..4326fa56b 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -4,6 +4,7 @@ * Copyright (c) 2006 Junio C Hamano */ #include "cache.h" +#include "lockfile.h" #include "color.h" #include "commit.h" #include "blob.h" diff --git a/builtin/gc.c b/builtin/gc.c index ced1456e1..005adbebe 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -11,7 +11,7 @@ */ #include "builtin.h" -#include "cache.h" +#include "lockfile.h" #include "parse-options.h" #include "run-command.h" #include "sigchain.h" diff --git a/builtin/merge.c b/builtin/merge.c index be07f2748..4513fadc5 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -9,6 +9,7 @@ #include "cache.h" #include "parse-options.h" #include "builtin.h" +#include "lockfile.h" #include "run-command.h" #include "diff.h" #include "refs.h" diff --git a/builtin/mv.c b/builtin/mv.c index 8883baa90..563d05ba1 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -3,8 +3,8 @@ * * Copyright (C) 2006 Johannes Schindelin */ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "dir.h" #include "cache-tree.h" #include "string-list.h" diff --git a/builtin/read-tree.c b/builtin/read-tree.c index e7e1c33a7..43b47f72f 100644 --- a/builtin/read-tree.c +++ b/builtin/read-tree.c @@ -5,6 +5,7 @@ */ #include "cache.h" +#include "lockfile.h" #include "object.h" #include "tree.h" #include "tree-walk.h" diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index daf0600ca..10fa25d7b 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1,4 +1,5 @@ #include "builtin.h" +#include "lockfile.h" #include "pack.h" #include "refs.h" #include "pkt-line.h" diff --git a/builtin/reflog.c b/builtin/reflog.c index 7c78b15c1..b6388f75b 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -1,5 +1,5 @@ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "commit.h" #include "refs.h" #include "dir.h" diff --git a/builtin/reset.c b/builtin/reset.c index 855d478e3..4c08ddc1c 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -8,6 +8,7 @@ * Copyright (c) 2005, 2006 Linus Torvalds and Junio C Hamano */ #include "builtin.h" +#include "lockfile.h" #include "tag.h" #include "object.h" #include "commit.h" diff --git a/builtin/rm.c b/builtin/rm.c index 2b61d3bd4..d8a9c86dd 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -3,8 +3,8 @@ * * Copyright (C) Linus Torvalds 2006 */ -#include "cache.h" #include "builtin.h" +#include "lockfile.h" #include "dir.h" #include "cache-tree.h" #include "tree-walk.h" diff --git a/builtin/update-index.c b/builtin/update-index.c index 6c9598891..b0e3dc910 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -4,6 +4,7 @@ * Copyright (C) Linus Torvalds, 2005 */ #include "cache.h" +#include "lockfile.h" #include "quote.h" #include "cache-tree.h" #include "tree-walk.h" diff --git a/bundle.c b/bundle.c index b2b89fe86..891a3cacd 100644 --- a/bundle.c +++ b/bundle.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "bundle.h" #include "object.h" #include "commit.h" diff --git a/cache-tree.c b/cache-tree.c index 75a54fdc7..215202c42 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "tree.h" #include "tree-walk.h" #include "cache-tree.h" diff --git a/cache.h b/cache.h index d19e57ff6..b71ceb2d8 100644 --- a/cache.h +++ b/cache.h @@ -570,36 +570,11 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs update" */ extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); -/* String appended to a filename to derive the lockfile name: */ -#define LOCK_SUFFIX ".lock" -#define LOCK_SUFFIX_LEN 5 - -struct lock_file { - struct lock_file *volatile next; - volatile sig_atomic_t active; - volatile int fd; - volatile pid_t owner; - char on_list; - struct strbuf filename; -}; -#define LOCK_DIE_ON_ERROR 1 -#define LOCK_NO_DEREF 2 -extern int unable_to_lock_error(const char *path, int err); -extern void unable_to_lock_message(const char *path, int err, - struct strbuf *buf); -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 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 *); -extern int reopen_lock_file(struct lock_file *); extern void update_index_if_able(struct index_state *, struct lock_file *); extern int hold_locked_index(struct lock_file *, int); extern void set_alternate_index_output(const char *); -extern int close_lock_file(struct lock_file *); -extern void rollback_lock_file(struct lock_file *); + extern int delete_ref(const char *, const unsigned char *sha1, int delopt); /* Environment bits from configuration mechanism */ diff --git a/config.c b/config.c index 21107797d..c31d4d248 100644 --- a/config.c +++ b/config.c @@ -6,6 +6,7 @@ * */ #include "cache.h" +#include "lockfile.h" #include "exec_cmd.h" #include "strbuf.h" #include "quote.h" diff --git a/credential-store.c b/credential-store.c index f9146e576..d435514cb 100644 --- a/credential-store.c +++ b/credential-store.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "credential.h" #include "string-list.h" #include "parse-options.h" diff --git a/fast-import.c b/fast-import.c index 783c6840b..deadc33f9 100644 --- a/fast-import.c +++ b/fast-import.c @@ -153,6 +153,7 @@ Format of STDIN stream: #include "builtin.h" #include "cache.h" +#include "lockfile.h" #include "object.h" #include "blob.h" #include "tree.h" diff --git a/fetch-pack.c b/fetch-pack.c index 7487aa730..655ee6425 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "refs.h" #include "pkt-line.h" #include "commit.h" diff --git a/lockfile.c b/lockfile.c index 63f4e94bc..d27e61caf 100644 --- a/lockfile.c +++ b/lockfile.c @@ -2,59 +2,9 @@ * Copyright (c) 2005, Junio C Hamano */ #include "cache.h" +#include "lockfile.h" #include "sigchain.h" -/* - * File write-locks as used by Git. - * - * For an overview of how to use the lockfile API, please see - * - * Documentation/technical/api-lockfile.txt - * - * This module keeps track of all locked files in lock_file_list for - * use at cleanup. This list and the lock_file objects that comprise - * it must be kept in self-consistent states at all time, because the - * program can be interrupted any time by a signal, in which case the - * signal handler will walk through the list attempting to clean up - * any open lock files. - * - * A lockfile is owned by the process that created it. The lock_file - * object has an "owner" field that records its owner. This field is - * used to prevent a forked process from closing a lockfile created by - * its parent. - * - * The possible states of a lock_file object are as follows: - * - * - Uninitialized. In this state the object's on_list field must be - * zero but the rest of its contents need not be initialized. As - * soon as the object is used in any way, it is irrevocably - * registered in the lock_file_list, and on_list is set. - * - * - Locked, lockfile open (after hold_lock_file_for_update(), - * hold_lock_file_for_append(), or reopen_lock_file()). In this - * state: - * - the lockfile exists - * - active is set - * - filename holds the filename of the lockfile - * - fd holds a file descriptor open for writing to the lockfile - * - owner holds the PID of the process that locked the file - * - * - Locked, lockfile closed (after successful close_lock_file()). - * Same as the previous state, except that the lockfile is closed - * and fd is -1. - * - * - Unlocked (after commit_lock_file(), commit_lock_file_to(), - * rollback_lock_file(), a failed attempt to lock, or a failed - * close_lock_file()). In this state: - * - active is unset - * - filename is empty (usually, though there are transitory - * states in which this condition doesn't hold). Client code should - * *not* rely on the filename being empty in this state. - * - fd is -1 - * - the object is left registered in the lock_file_list, and - * on_list is set. - */ - static struct lock_file *volatile lock_file_list; static void remove_lock_files(void) diff --git a/lockfile.h b/lockfile.h new file mode 100644 index 000000000..9059e8958 --- /dev/null +++ b/lockfile.h @@ -0,0 +1,84 @@ +#ifndef LOCKFILE_H +#define LOCKFILE_H + +/* + * File write-locks as used by Git. + * + * For an overview of how to use the lockfile API, please see + * + * Documentation/technical/api-lockfile.txt + * + * This module keeps track of all locked files in lock_file_list for + * use at cleanup. This list and the lock_file objects that comprise + * it must be kept in self-consistent states at all time, because the + * program can be interrupted any time by a signal, in which case the + * signal handler will walk through the list attempting to clean up + * any open lock files. + * + * A lockfile is owned by the process that created it. The lock_file + * object has an "owner" field that records its owner. This field is + * used to prevent a forked process from closing a lockfile created by + * its parent. + * + * The possible states of a lock_file object are as follows: + * + * - Uninitialized. In this state the object's on_list field must be + * zero but the rest of its contents need not be initialized. As + * soon as the object is used in any way, it is irrevocably + * registered in the lock_file_list, and on_list is set. + * + * - Locked, lockfile open (after hold_lock_file_for_update(), + * hold_lock_file_for_append(), or reopen_lock_file()). In this + * state: + * - the lockfile exists + * - active is set + * - filename holds the filename of the lockfile + * - fd holds a file descriptor open for writing to the lockfile + * - owner holds the PID of the process that locked the file + * + * - Locked, lockfile closed (after successful close_lock_file()). + * Same as the previous state, except that the lockfile is closed + * and fd is -1. + * + * - Unlocked (after commit_lock_file(), commit_lock_file_to(), + * rollback_lock_file(), a failed attempt to lock, or a failed + * close_lock_file()). In this state: + * - active is unset + * - filename is empty (usually, though there are transitory + * states in which this condition doesn't hold). Client code should + * *not* rely on the filename being empty in this state. + * - fd is -1 + * - the object is left registered in the lock_file_list, and + * on_list is set. + */ + +struct lock_file { + struct lock_file *volatile next; + volatile sig_atomic_t active; + volatile int fd; + volatile pid_t owner; + char on_list; + struct strbuf filename; +}; + +/* String appended to a filename to derive the lockfile name: */ +#define LOCK_SUFFIX ".lock" +#define LOCK_SUFFIX_LEN 5 + +#define LOCK_DIE_ON_ERROR 1 +#define LOCK_NO_DEREF 2 + +extern int unable_to_lock_error(const char *path, int err); +extern void unable_to_lock_message(const char *path, int err, + struct strbuf *buf); +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 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 *); +extern int reopen_lock_file(struct lock_file *); +extern int close_lock_file(struct lock_file *); +extern void rollback_lock_file(struct lock_file *); + +#endif /* LOCKFILE_H */ diff --git a/merge-recursive.c b/merge-recursive.c index 8ad4be897..4b0884b7b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -5,6 +5,7 @@ */ #include "advice.h" #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "commit.h" #include "blob.h" diff --git a/merge.c b/merge.c index 74ced7f70..fcff632bd 100644 --- a/merge.c +++ b/merge.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "commit.h" #include "run-command.h" #include "resolve-undo.h" diff --git a/read-cache.c b/read-cache.c index 9f137e7f2..8f3e9eb31 100644 --- a/read-cache.c +++ b/read-cache.c @@ -5,6 +5,7 @@ */ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "lockfile.h" #include "cache-tree.h" #include "refs.h" #include "dir.h" diff --git a/refs.c b/refs.c index e40c47edc..1d73f1daf 100644 --- a/refs.c +++ b/refs.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "refs.h" #include "object.h" #include "tag.h" diff --git a/rerere.c b/rerere.c index 20b18add4..1b0555f1a 100644 --- a/rerere.c +++ b/rerere.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "string-list.h" #include "rerere.h" #include "xdiff-interface.h" diff --git a/sequencer.c b/sequencer.c index 5e8a20747..1b9a35e58 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "sequencer.h" #include "dir.h" #include "object.h" diff --git a/sha1_file.c b/sha1_file.c index c08c0cbea..46ff273e2 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -8,6 +8,7 @@ */ #include "cache.h" #include "string-list.h" +#include "lockfile.h" #include "delta.h" #include "pack.h" #include "blob.h" diff --git a/shallow.c b/shallow.c index 4919baf77..bd7569e81 100644 --- a/shallow.c +++ b/shallow.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "commit.h" #include "tag.h" #include "pkt-line.h" diff --git a/test-scrap-cache-tree.c b/test-scrap-cache-tree.c index 9ebcbca9d..6efee31a4 100644 --- a/test-scrap-cache-tree.c +++ b/test-scrap-cache-tree.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "lockfile.h" #include "tree.h" #include "cache-tree.h"