{"thread":{"id":"36409","subject":"[PATCH v3 00/25] Lockfile correctness and refactoring","startedAt":"2014-04-14T13:54:30Z","lastAt":"2014-04-16T19:50:56Z","messageCount":32,"participants":["Michael Haggerty","Johannes Sixt","Torsten Bögershausen"],"isPatch":true,"patchVersion":3,"patchTotal":25},"messages":[{"id":"238823","messageId":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":null,"subject":"[PATCH v3 00/25] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:30Z","receivedAt":"2014-04-14T13:54:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Round v3. Thanks to Johannes Sixt and Peff for feedback on v2. This\nversion addresses all issues raised for v1 [1] and v2 [2].\n\nChanges since v2:\n\n* Instead of keeping track of whether a lock_file object is active via\n a new bit in a flags bitmask, store it in a separate volatile\n sig_atomic_t struct member. This makes it a little less undefined\n when this field is accessed by the signal handler.\n\n* Add some other \"volatile\" qualifiers to values used by the signal\n handler.\n\n* Define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN in cache.h and use\n them both inside and outside lockfile.c. (In v2, only\n LOCK_SUFFIX_LEN was defined and it was only used within lockfile.c,\n because the other potential users in refs.c were rewritten anyway.\n But that rewriting is no longer included in the patch series, so it\n makes sense to define these constants as part of the public lockfile\n API.)\n\n* Swap order of first two patches because the documentation lists\n unable_to_lock_die() under its new name.\n\n* die() (instead of NOP) if commit_lock_file() is called for an\n unlocked lock_file object.\n\n* Rebase to current master (there were no conflicts).\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/245609\n[2] http://thread.gmane.org/gmane.comp.version-control.git/245801\n\nMichael Haggerty (25):\n unable_to_lock_die(): rename function from unable_to_lock_index_die()\n api-lockfile: expand the documentation\n rollback_lock_file(): do not clear filename redundantly\n rollback_lock_file(): set fd to -1\n lockfile: unlock file if lockfile permissions cannot be adjusted\n hold_lock_file_for_append(): release lock on errors\n lock_file(): always add lock_file object to lock_file_list\n lockfile.c: document the various states of lock_file objects\n cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN\n delete_ref_loose(): don't muck around in the lock_file's filename\n prepare_index(): declare return value to be (const char *)\n write_packed_entry_fn(): convert cb_data into a (const int *)\n lock_file(): exit early if lockfile cannot be opened\n remove_lock_file(): call rollback_lock_file()\n commit_lock_file(): inline temporary variable\n commit_lock_file(): die() if called for unlocked lockfile object\n lockfile: avoid transitory invalid states\n struct lock_file: declare some fields volatile\n try_merge_strategy(): remove redundant lock_file allocation\n try_merge_strategy(): use a statically-allocated lock_file object\n commit_lock_file(): use a strbuf to manage temporary space\n Change lock_file::filename into a strbuf\n resolve_symlink(): use a strbuf for internal scratch space\n resolve_symlink(): take a strbuf parameter\n trim_last_path_elm(): replace last_path_elm()\n\n Documentation/technical/api-lockfile.txt | 40 ++++-\n builtin/commit.c | 16 +-\n builtin/merge.c | 15 +-\n builtin/reflog.c | 2 +-\n builtin/update-index.c | 2 +-\n cache.h | 15 +-\n config.c | 6 +-\n lockfile.c | 275 ++++++++++++++++++-------------\n refs.c | 29 ++--\n shallow.c | 6 +-\n 10 files changed, 247 insertions(+), 159 deletions(-)\n\n-- \n1.9.1\n"},{"id":"238824","messageId":"1397483695-10888-2-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 01/25] unable_to_lock_die(): rename function from unable_to_lock_index_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:31Z","receivedAt":"2014-04-14T13:54:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function is used for other things besides the index, so rename it\naccordingly.\n\nSuggested-by: Jeff King \nSigned-off-by: Michael Haggerty \n---\n builtin/update-index.c | 2 +-\n cache.h | 2 +-\n lockfile.c | 6 +++---\n refs.c | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex ba54e19..0bab053 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -919,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n-\t\t\tunable_to_lock_index_die(get_index_file(), lock_error);\n+\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n \t\t}\n \t\tif (write_cache(newfd, active_cache, active_nr) ||\n \t\t commit_locked_index(lock_file))\ndiff --git a/cache.h b/cache.h\nindex 107ac61..3a873a4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -543,7 +543,7 @@ struct lock_file {\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\n extern int unable_to_lock_error(const char *path, int err);\n-extern NORETURN void unable_to_lock_index_die(const char *path, int err);\n+extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..d4bfb3f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -181,7 +181,7 @@ int unable_to_lock_error(const char *path, int err)\n \treturn -1;\n }\n \n-NORETURN void unable_to_lock_index_die(const char *path, int err)\n+NORETURN void unable_to_lock_die(const char *path, int err)\n {\n \tdie(\"%s\", unable_to_lock_message(path, err));\n }\n@@ -190,7 +190,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd = lock_file(lk, path, flags);\n \tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_index_die(path, errno);\n+\t\tunable_to_lock_die(path, errno);\n \treturn fd;\n }\n \n@@ -201,7 +201,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_index_die(path, errno);\n+\t\t\tunable_to_lock_die(path, errno);\n \t\treturn fd;\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 28d5eca..67fe8b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2118,7 +2118,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse\n-\t\t\tunable_to_lock_index_die(ref_file, errno);\n+\t\t\tunable_to_lock_die(ref_file, errno);\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n1.9.1\n"},{"id":"238846","messageId":"1397483695-10888-3-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 02/25] api-lockfile: expand the documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:32Z","receivedAt":"2014-04-14T13:54:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document a couple more functions and the flags argument as used by\nhold_lock_file_for_update() and hold_lock_file_for_append().\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 36 +++++++++++++++++++++++++++++---\n 1 file changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex dd89404..b53e300 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -28,9 +28,39 @@ hold_lock_file_for_update::\n \tthe final destination (e.g. `$GIT_DIR/index`) and a flag\n \t`die_on_error`. Attempt to create a lockfile for the\n \tdestination and return the file descriptor for writing\n-\tto the file. If `die_on_error` flag is true, it dies if\n-\ta lock is already taken for the file; otherwise it\n-\treturns a negative integer to the caller on failure.\n+\tto the file. The flags parameter is a combination of\n++\n+--\n+LOCK_NODEREF::\n+\n+\tUsually symbolic links in path are resolved in path and the\n+\tlockfile is created by adding \".lock\" to the resolved path;\n+\thowever, if `LOCK_NODEREF` is set, then the lockfile is\n+\tcreated by adding \".lock\" to the path argument itself.\n+\n+LOCK_DIE_ON_ERROR::\n+\n+\tIf a lock is already taken for the file, `die()` with an error\n+\tmessage. If this option is not specified, return a negative\n+\tinteger to the caller on failure.\n+--\n+\n+hold_lock_file_for_append::\n+\n+\tLike `hold_lock_file_for_update()`, except that additionally\n+\tthe existing contents of the file (if any) are copied to the\n+\tlockfile and its write pointer is positioned at the end of the\n+\tfile before returning.\n+\n+unable_to_lock_error::\n+\n+\tEmit an error describing that there was an error locking the\n+\tspecified path. The err parameter should be the errno of the\n+\tproblem that caused the failure.\n+\n+unable_to_lock_die::\n+\n+\tLike `unable_to_lock_error()`, but also `die()`.\n \n commit_lock_file::\n \n-- \n1.9.1\n"},{"id":"238847","messageId":"1397483695-10888-4-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 03/25] rollback_lock_file(): do not clear filename redundantly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:33Z","receivedAt":"2014-04-14T13:54:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is only necessary to clear the lock_file's filename field if it was\nnot already clear.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex d4bfb3f..7701267 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -277,6 +277,6 @@ void rollback_lock_file(struct lock_file *lk)\n \t\tif (lk->fd >= 0)\n \t\t\tclose(lk->fd);\n \t\tunlink_or_warn(lk->filename);\n+\t\tlk->filename[0] = 0;\n \t}\n-\tlk->filename[0] = 0;\n }\n-- \n1.9.1\n"},{"id":"238825","messageId":"1397483695-10888-5-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 04/25] rollback_lock_file(): set fd to -1","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:34Z","receivedAt":"2014-04-14T13:54:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When rolling back the lockfile, call close_lock_file() so that the\nlock_file's fd field gets set back to -1. This keeps the lock_file\nobject in a valid state, which is important because these objects are\nallowed to be reused.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 7701267..1122542 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -275,7 +275,7 @@ void rollback_lock_file(struct lock_file *lk)\n {\n \tif (lk->filename[0]) {\n \t\tif (lk->fd >= 0)\n-\t\t\tclose(lk->fd);\n+\t\t\tclose_lock_file(lk);\n \t\tunlink_or_warn(lk->filename);\n \t\tlk->filename[0] = 0;\n \t}\n-- \n1.9.1\n"},{"id":"238848","messageId":"1397483695-10888-6-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 05/25] lockfile: unlock file if lockfile permissions cannot be adjusted","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:35Z","receivedAt":"2014-04-14T13:54:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the call to adjust_shared_perm() fails, lock_file returns -1, which\nto the caller looks like any other failure to lock the file. So in\nthis case, roll back the lockfile before returning so that the lock\nfile is deleted immediately and the lockfile object is left in a\npredictable state (namely, unlocked). Previously, the lockfile was\nretained until process cleanup in this situation.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1122542..b101f77 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -148,9 +148,11 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\tlock_file_list = lk;\n \t\t\tlk->on_list = 1;\n \t\t}\n-\t\tif (adjust_shared_perm(lk->filename))\n-\t\t\treturn error(\"cannot fix permission bits on %s\",\n-\t\t\t\t lk->filename);\n+\t\tif (adjust_shared_perm(lk->filename)) {\n+\t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\t\trollback_lock_file(lk);\n+\t\t\treturn -1;\n+\t\t}\n \t}\n \telse\n \t\tlk->filename[0] = 0;\n-- \n1.9.1\n"},{"id":"238827","messageId":"1397483695-10888-7-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 06/25] hold_lock_file_for_append(): release lock on errors","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:36Z","receivedAt":"2014-04-14T13:54:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If there is an error copying the old contents to the lockfile, roll\nback the lockfile before exiting so that the lockfile is not held\nuntil process cleanup.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex b101f77..e78a35f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -212,13 +212,13 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t\tif (errno != ENOENT) {\n \t\t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\t\tdie(\"cannot open '%s' for copying\", path);\n-\t\t\tclose(fd);\n+\t\t\trollback_lock_file(lk);\n \t\t\treturn error(\"cannot open '%s' for copying\", path);\n \t\t}\n \t} else if (copy_fd(orig_fd, fd)) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n-\t\tclose(fd);\n+\t\trollback_lock_file(lk);\n \t\treturn -1;\n \t}\n \treturn fd;\n-- \n1.9.1\n"},{"id":"238826","messageId":"1397483695-10888-8-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 07/25] lock_file(): always add lock_file object to lock_file_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:37Z","receivedAt":"2014-04-14T13:54:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It used to be that if locking failed, lock_file() usually did not\nregister the lock_file object in lock_file_list but sometimes it did.\nThis confusion was compounded if lock_file() was called via\nhold_lock_file_for_append(), which has its own failure modes.\n\nThe ambiguity didn't have any ill effects, because lock_file objects\ncannot be removed from the lock_file_list anyway. But it is\nunnecessary to leave this behavior inconsistent.\n\nSo change lock_file() to *always* ensure that the lock_file object is\nregistered in lock_file_list regardless of whether an error occurs.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex e78a35f..120a6d6 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -130,6 +130,22 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t */\n \tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n \n+\tif (!lock_file_list) {\n+\t\t/* One-time initialization */\n+\t\tsigchain_push_common(remove_lock_file_on_signal);\n+\t\tatexit(remove_lock_file);\n+\t}\n+\n+\tif (!lk->on_list) {\n+\t\t/* Initialize *lk and add it to lock_file_list: */\n+\t\tlk->fd = -1;\n+\t\tlk->owner = 0;\n+\t\tlk->on_list = 1;\n+\t\tlk->filename[0] = 0;\n+\t\tlk->next = lock_file_list;\n+\t\tlock_file_list = lk;\n+\t}\n+\n \tif (strlen(path) >= max_path_len)\n \t\treturn -1;\n \tstrcpy(lk->filename, path);\n@@ -138,16 +154,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n-\t\tif (!lock_file_list) {\n-\t\t\tsigchain_push_common(remove_lock_file_on_signal);\n-\t\t\tatexit(remove_lock_file);\n-\t\t}\n \t\tlk->owner = getpid();\n-\t\tif (!lk->on_list) {\n-\t\t\tlk->next = lock_file_list;\n-\t\t\tlock_file_list = lk;\n-\t\t\tlk->on_list = 1;\n-\t\t}\n \t\tif (adjust_shared_perm(lk->filename)) {\n \t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n \t\t\trollback_lock_file(lk);\n-- \n1.9.1\n"},{"id":"238844","messageId":"1397483695-10888-9-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 08/25] lockfile.c: document the various states of lock_file objects","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:38Z","receivedAt":"2014-04-14T13:54:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Document the valid states of lock_file objects, how they get into each\nstate, and how the state is encoded in the object's fields.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 51 insertions(+)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 120a6d6..8f6652c 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -4,6 +4,57 @@\n #include \"cache.h\"\n #include \"sigchain.h\"\n \n+/*\n+ * File write-locks as used by Git.\n+ *\n+ * When a file at $FILENAME needs to be written, it is done as\n+ * follows:\n+ *\n+ * 1. Obtain a lock on the file by creating a lockfile at path\n+ * $FILENAME.lock. The file is opened for read/write using O_CREAT\n+ * and O_EXCL mode to ensure that it doesn't already exist. Such a\n+ * lock file is respected by writers *but not by readers*.\n+ *\n+ * Usually, if $FILENAME is a symlink, then it is resolved, and the\n+ * file ultimately pointed to is the one that is locked and later\n+ * replaced. However, if LOCK_NODEREF is used, then $FILENAME\n+ * itself is locked and later replaced, even if it is a symlink.\n+ *\n+ * 2. Write the new file contents to the lockfile.\n+ *\n+ * 3. Move the lockfile to its final destination using rename(2).\n+ *\n+ * Instead of (3), the change can be rolled back by deleting lockfile.\n+ *\n+ * This module keeps track of all locked files in lock_file_list.\n+ * When the first file is locked, it registers an atexit(3) handler;\n+ * when the program exits, the handler rolls back any files that have\n+ * been locked but were never committed or rolled back.\n+ *\n+ * A lock_file object can be in several states:\n+ *\n+ * - Uninitialized. In this state the object's on_list field must be\n+ * zero but the rest of its contents need not be initialized. As\n+ * soon as the object is used in any way, it is irrevocably\n+ * registered in the lock_file_list, and on_list is set.\n+ *\n+ * - Locked, lockfile open (after hold_lock_file_for_update() or\n+ * hold_lock_file_for_append()). In this state, the lockfile\n+ * exists, filename holds the filename of the lockfile, fd holds a\n+ * file descriptor open for writing to the lockfile, and owner holds\n+ * the PID of the process that locked the file.\n+ *\n+ * - Locked, lockfile closed (after close_lock_file()). Same as the\n+ * previous state, except that the lockfile is closed and fd is -1.\n+ *\n+ * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n+ * failed attempt to lock). In this state, filename[0] == '\\0' and\n+ * fd is -1. The object is left registered in the lock_file_list,\n+ * and on_list is set.\n+ *\n+ * See Documentation/api-lockfile.txt for more information.\n+ */\n+\n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n \n-- \n1.9.1\n"},{"id":"238829","messageId":"1397483695-10888-10-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 09/25] cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:39Z","receivedAt":"2014-04-14T13:54:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There are a few places that use these values, so define constants for\nthem.\n\nSigned-off-by: Michael Haggerty \n---\nIf I were confident that it would always be inlined, I would have\nused\n\n #define LOCK_SUFFIX_LEN strlen(LOCK_SUFFIX)\n\nOr maybe even have omitted this constant and write strlen(LOCK_SUFFIX)\ninline where it is needed instead. But I'm not so I didn't.\n\nIt's possible I missed other code that uses \".lock\" or the magic\nnumber 5, but these are the only ones I found.\n\n cache.h | 4 ++++\n lockfile.c | 12 ++++++------\n refs.c | 7 ++++---\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3a873a4..825cd0a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -533,6 +533,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n #define REFRESH_IN_PORCELAIN\t0x0020\t/* user friendly output, not \"needs update\" */\n extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);\n \n+/* String appended to a filename to derive the lockfile name: */\n+#define LOCK_SUFFIX \".lock\"\n+#define LOCK_SUFFIX_LEN 5\n+\n struct lock_file {\n \tstruct lock_file *next;\n \tint fd;\ndiff --git a/lockfile.c b/lockfile.c\nindex 8f6652c..1ce0e87 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -172,14 +172,14 @@ static char *resolve_symlink(char *p, size_t s)\n \treturn p;\n }\n \n-\n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n \t/*\n-\t * subtract 5 from size to make sure there's room for adding\n-\t * \".lock\" for the lock file name\n+\t * subtract LOCK_SUFFIX_LEN from size to make sure there's\n+\t * room for adding \".lock\" for the lock file name:\n \t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n+\tstatic const size_t max_path_len = sizeof(lk->filename) -\n+\t\t\t\t\t LOCK_SUFFIX_LEN;\n \n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n@@ -202,7 +202,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tstrcpy(lk->filename, path);\n \tif (!(flags & LOCK_NODEREF))\n \t\tresolve_symlink(lk->filename, max_path_len);\n-\tstrcat(lk->filename, \".lock\");\n+\tstrcat(lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tlk->owner = getpid();\n@@ -296,7 +296,7 @@ int commit_lock_file(struct lock_file *lk)\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n \tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - 5; /* .lock */\n+\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n \tresult_file[i] = 0;\n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\ndiff --git a/refs.c b/refs.c\nindex 67fe8b7..2805b31 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -63,7 +63,8 @@ static int check_refname_component(const char *refname, int flags)\n \t\tif (refname[1] == '\\0')\n \t\t\treturn -1; /* Component equals \".\". */\n \t}\n-\tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n+\tif (cp - refname >= LOCK_SUFFIX_LEN &&\n+\t !memcmp(cp - LOCK_SUFFIX_LEN, LOCK_SUFFIX, LOCK_SUFFIX_LEN))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - refname;\n }\n@@ -2486,11 +2487,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n \t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - 5; /* .lock */\n+\t\tint err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n \n \t\tlock->lk->filename[i] = 0;\n \t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = '.';\n+\t\tlock->lk->filename[i] = LOCK_SUFFIX[0];\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n1.9.1\n"},{"id":"238843","messageId":"1397483695-10888-11-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 10/25] delete_ref_loose(): don't muck around in the lock_file's filename","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:40Z","receivedAt":"2014-04-14T13:54:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners. Especially since, if unlink_or_warn() failed, the\nmemory wasn't restored to its original contents.\n\nSo make our own copy to work with.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2805b31..20c483b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2486,12 +2486,15 @@ static int repack_without_ref(const char *refname)\n static int delete_ref_loose(struct ref_lock *lock, int flag)\n {\n \tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/* loose */\n-\t\tint err, i = strlen(lock->lk->filename) - LOCK_SUFFIX_LEN;\n-\n-\t\tlock->lk->filename[i] = 0;\n-\t\terr = unlink_or_warn(lock->lk->filename);\n-\t\tlock->lk->filename[i] = LOCK_SUFFIX[0];\n+\t\t/*\n+\t\t * loose. The loose file name is the same as the\n+\t\t * lockfile name, minus \".lock\":\n+\t\t */\n+\t\tchar *loose_filename = xmemdupz(\n+\t\t\t\tlock->lk->filename,\n+\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n+\t\tint err = unlink_or_warn(loose_filename);\n+\t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n \t\t\treturn 1;\n \t}\n-- \n1.9.1\n"},{"id":"238845","messageId":"1397483695-10888-12-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 11/25] prepare_index(): declare return value to be (const char *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:41Z","receivedAt":"2014-04-14T13:54:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Declare the return value to be const to make it clear that we aren't\ngiving callers permission to write over the string that it points at.\n(The return value is the filename field of a struct lock_file, which\ncan be used by a signal handler at any time and therefore shouldn't be\ntampered with.)\n\nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 9cfef6c..8ffb3ef 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -302,8 +302,8 @@ static void refresh_cache_or_die(int refresh_flags)\n \t\tdie_resolve_conflict(\"commit\");\n }\n \n-static char *prepare_index(int argc, const char **argv, const char *prefix,\n-\t\t\t const struct commit *current_head, int is_status)\n+static const char *prepare_index(int argc, const char **argv, const char *prefix,\n+\t\t\t\t const struct commit *current_head, int is_status)\n {\n \tint fd;\n \tstruct string_list partial;\n-- \n1.9.1\n"},{"id":"238840","messageId":"1397483695-10888-13-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 12/25] write_packed_entry_fn(): convert cb_data into a (const int *)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:42Z","receivedAt":"2014-04-14T13:54:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This makes it obvious that we have no plans to change the integer\npointed to, which is actually the fd field from a struct lock_file.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 20c483b..cb2f825 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2177,7 +2177,7 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n */\n static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tint *fd = cb_data;\n+\tconst int *fd = cb_data;\n \tenum peel_status peel_status = peel_entry(entry, 0);\n \n \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n-- \n1.9.1\n"},{"id":"238828","messageId":"1397483695-10888-14-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 13/25] lock_file(): exit early if lockfile cannot be opened","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:43Z","receivedAt":"2014-04-14T13:54:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a bit easier to read than the old version, which nested part\nof the non-error code in an \"if\" block.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 1ce0e87..ba791d5 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -204,16 +204,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tresolve_symlink(lk->filename, max_path_len);\n \tstrcat(lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n-\tif (0 <= lk->fd) {\n-\t\tlk->owner = getpid();\n-\t\tif (adjust_shared_perm(lk->filename)) {\n-\t\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n-\t\t\trollback_lock_file(lk);\n-\t\t\treturn -1;\n-\t\t}\n-\t}\n-\telse\n+\tif (lk->fd < 0) {\n \t\tlk->filename[0] = 0;\n+\t\treturn -1;\n+\t}\n+\tlk->owner = getpid();\n+\tif (adjust_shared_perm(lk->filename)) {\n+\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\t\trollback_lock_file(lk);\n+\t\treturn -1;\n+\t}\n \treturn lk->fd;\n }\n \n-- \n1.9.1\n"},{"id":"238842","messageId":"1397483695-10888-15-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 14/25] remove_lock_file(): call rollback_lock_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:44Z","receivedAt":"2014-04-14T13:54:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It does just what we need.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex ba791d5..477bf4b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -63,12 +63,8 @@ static void remove_lock_file(void)\n \tpid_t me = getpid();\n \n \twhile (lock_file_list) {\n-\t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->filename[0]) {\n-\t\t\tif (lock_file_list->fd >= 0)\n-\t\t\t\tclose(lock_file_list->fd);\n-\t\t\tunlink_or_warn(lock_file_list->filename);\n-\t\t}\n+\t\tif (lock_file_list->owner == me)\n+\t\t\trollback_lock_file(lock_file_list);\n \t\tlock_file_list = lock_file_list->next;\n \t}\n }\n-- \n1.9.1\n"},{"id":"238838","messageId":"1397483695-10888-16-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 15/25] commit_lock_file(): inline temporary variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:45Z","receivedAt":"2014-04-14T13:54:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n lockfile.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 477bf4b..664b0c3 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -288,12 +288,14 @@ int close_lock_file(struct lock_file *lk)\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n-\tsize_t i;\n+\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n+\n \tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - LOCK_SUFFIX_LEN; /* .lock */\n-\tresult_file[i] = 0;\n+\t/* remove \".lock\": */\n+\tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n+\n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\n \tlk->filename[0] = 0;\n-- \n1.9.1\n"},{"id":"238830","messageId":"1397483695-10888-17-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 16/25] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:46Z","receivedAt":"2014-04-14T13:54:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It was previously a bug to call commit_lock_file() with a lock_file\nobject that was not active (an illegal access would happen within the\nfunction). It was presumably never done, but this would be an easy\nprogramming error to overlook. So before continuing, do a consistency\ncheck that the lock_file object really is locked.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/technical/api-lockfile.txt | 4 +++-\n lockfile.c | 3 +++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex b53e300..9a94ead 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -68,7 +68,9 @@ commit_lock_file::\n \twith an earlier call to `hold_lock_file_for_update()`,\n \tclose the file descriptor and rename the lockfile to its\n \tfinal destination. Returns 0 upon success, a negative\n-\tvalue on failure to close(2) or rename(2).\n+\tvalue on failure to close(2) or rename(2). It is a bug to\n+\tcall `commit_lock_file()` for a `lock_file` object that is not\n+\tcurrently locked.\n \n rollback_lock_file::\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 664b0c3..1453a7a 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -292,6 +292,9 @@ int commit_lock_file(struct lock_file *lk)\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n \n+\tif (!lk->filename[0])\n+\t\tdie(\"BUG: attempt to commit unlocked object\");\n+\n \tstrcpy(result_file, lk->filename);\n \t/* remove \".lock\": */\n \tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n-- \n1.9.1\n"},{"id":"238839","messageId":"1397483695-10888-18-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 17/25] lockfile: avoid transitory invalid states","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:47Z","receivedAt":"2014-04-14T13:54:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Because remove_lock_file() can be called any time by the signal\nhandler, it is important that any lock_file objects that are in the\nlock_file_list are always in a valid state. And since lock_file\nobjects are often reused (but are never removed from lock_file_list),\nthat means we have to be careful whenever mutating a lock_file object\nto always keep it in a well-defined state.\n\nThis was formerly not the case, because part of the state was recorded\nby whether lk->filename was the empty string or a valid filename. It\nis wrong to assume that this string can be updated atomically; for\nexample, even\n\n strcpy(lk->filename, value)\n\nis unsafe. But the old code was even more reckless; for example,\n\n strcpy(lk->filename, path);\n if (!(flags & LOCK_NODEREF))\n resolve_symlink(lk->filename, max_path_len);\n strcat(lk->filename, \".lock\");\n\nDuring the call to resolve_symlink(), lk->filename contained the name\nof the file that was being locked, not the name of the lockfile. If a\nsignal would have been raised during that interval, then the signal\nhandler would have deleted the valuable file!\n\nWe could probably continue to use the filename field to encode the\nstate by being careful to write characters 1..N-1 of the filename\nfirst, and then overwrite the NUL at filename[0] with the first\ncharacter of the filename, but that would be awkward and error-prone.\n\nSo, instead of using the filename field to determine whether the\nlock_file object is active, add a new field \"lock_file::active\" for\nthis purpose. Be careful to set this field only when filename really\ncontains the name of a file that should be deleted on cleanup.\n\nHelped-by: Johannes Sixt \nSigned-off-by: Michael Haggerty \n---\n cache.h | 1 +\n lockfile.c | 45 +++++++++++++++++++++++++++++++++------------\n 2 files changed, 34 insertions(+), 12 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 825cd0a..b7af173 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -539,6 +539,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n \n struct lock_file {\n \tstruct lock_file *next;\n+\tvolatile sig_atomic_t active;\n \tint fd;\n \tpid_t owner;\n \tchar on_list;\ndiff --git a/lockfile.c b/lockfile.c\nindex 1453a7a..50a0541 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -27,11 +27,14 @@\n * Instead of (3), the change can be rolled back by deleting lockfile.\n *\n * This module keeps track of all locked files in lock_file_list.\n- * When the first file is locked, it registers an atexit(3) handler;\n- * when the program exits, the handler rolls back any files that have\n- * been locked but were never committed or rolled back.\n+ * When the first file is locked, it registers an atexit(3) handler\n+ * and a signal handler; when the program exits, the handler rolls\n+ * back any files that have been locked but were never committed or\n+ * rolled back.\n *\n- * A lock_file object can be in several states:\n+ * Because the signal handler can be called at any time, a lock_file\n+ * object must always be in a well-defined state. The possible states\n+ * are as follows:\n *\n * - Uninitialized. In this state the object's on_list field must be\n * zero but the rest of its contents need not be initialized. As\n@@ -39,18 +42,29 @@\n * registered in the lock_file_list, and on_list is set.\n *\n * - Locked, lockfile open (after hold_lock_file_for_update() or\n- * hold_lock_file_for_append()). In this state, the lockfile\n- * exists, filename holds the filename of the lockfile, fd holds a\n- * file descriptor open for writing to the lockfile, and owner holds\n- * the PID of the process that locked the file.\n+ * hold_lock_file_for_append()). In this state:\n+ * - the lockfile exists\n+ * - active is set\n+ * - filename holds the filename of the lockfile\n+ * - fd holds a file descriptor open for writing to the lockfile\n+ * - owner holds the PID of the process that locked the file\n *\n- * - Locked, lockfile closed (after close_lock_file()). Same as the\n- * previous state, except that the lockfile is closed and fd is -1.\n+ * - Locked, lockfile closed (after close_lock_file() or an\n+ * unsuccessful commit_lock_file()). Same as the previous state,\n+ * except that the lockfile is closed and fd is -1.\n *\n * - Unlocked (after commit_lock_file(), rollback_lock_file(), or a\n * failed attempt to lock). In this state, filename[0] == '\\0' and\n * fd is -1. The object is left registered in the lock_file_list,\n * and on_list is set.\n+ * - Unlocked (after rollback_lock_file(), a successful\n+ * commit_lock_file(), or a failed attempt to lock). In this state:\n+ * - active is unset\n+ * - filename[0] == '\\0' (usually, though there are transitory states\n+ * in which this condition doesn't hold)\n+ * - fd is -1\n+ * - the object is left registered in the lock_file_list, and\n+ * on_list is set.\n *\n * See Documentation/api-lockfile.txt for more information.\n */\n@@ -183,9 +197,12 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tatexit(remove_lock_file);\n \t}\n \n+\tassert(!lk->active);\n+\n \tif (!lk->on_list) {\n \t\t/* Initialize *lk and add it to lock_file_list: */\n \t\tlk->fd = -1;\n+\t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n \t\tlk->filename[0] = 0;\n@@ -205,6 +222,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n+\tlk->active = 1;\n \tif (adjust_shared_perm(lk->filename)) {\n \t\terror(\"cannot fix permission bits on %s\", lk->filename);\n \t\trollback_lock_file(lk);\n@@ -292,7 +310,7 @@ int commit_lock_file(struct lock_file *lk)\n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n \n-\tif (!lk->filename[0])\n+\tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n \tstrcpy(result_file, lk->filename);\n@@ -301,6 +319,7 @@ int commit_lock_file(struct lock_file *lk)\n \n \tif (rename(lk->filename, result_file))\n \t\treturn -1;\n+\tlk->active = 0;\n \tlk->filename[0] = 0;\n \treturn 0;\n }\n@@ -325,6 +344,7 @@ int commit_locked_index(struct lock_file *lk)\n \t\t\treturn -1;\n \t\tif (rename(lk->filename, alternate_index_output))\n \t\t\treturn -1;\n+\t\tlk->active = 0;\n \t\tlk->filename[0] = 0;\n \t\treturn 0;\n \t}\n@@ -334,10 +354,11 @@ int commit_locked_index(struct lock_file *lk)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->filename[0]) {\n+\tif (lk->active) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n \t\tunlink_or_warn(lk->filename);\n+\t\tlk->active = 0;\n \t\tlk->filename[0] = 0;\n \t}\n }\n-- \n1.9.1\n"},{"id":"238831","messageId":"1397483695-10888-19-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 18/25] struct lock_file: declare some fields volatile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:48Z","receivedAt":"2014-04-14T13:54:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The function remove_lock_file_on_signal() is used as a signal handler.\nIt is not realistic to make the signal handler conform strictly to the\nC standard, which is very restrictive about what a signal handler is\nallowed to do. But let's increase the likelihood that it will work:\n\nThe lock_file_list global variable and several fields from struct\nlock_file are used by the signal handler. Declare those values\n\"volatile\" to increase the chance that the signal handler will see a\nvalid object state.\n\nSuggested-by: Johannes Sixt \nSigned-off-by: Michael Haggerty \n---\n cache.h | 6 +++---\n lockfile.c | 2 +-\n refs.c | 5 +++--\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b7af173..9019c7d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -538,10 +538,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n #define LOCK_SUFFIX_LEN 5\n \n struct lock_file {\n-\tstruct lock_file *next;\n+\tstruct lock_file *volatile next;\n \tvolatile sig_atomic_t active;\n-\tint fd;\n-\tpid_t owner;\n+\tvolatile int fd;\n+\tvolatile pid_t owner;\n \tchar on_list;\n \tchar filename[PATH_MAX];\n };\ndiff --git a/lockfile.c b/lockfile.c\nindex 50a0541..fce53f1 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -69,7 +69,7 @@\n * See Documentation/api-lockfile.txt for more information.\n */\n \n-static struct lock_file *lock_file_list;\n+static struct lock_file *volatile lock_file_list;\n static const char *alternate_index_output;\n \n static void remove_lock_file(void)\ndiff --git a/refs.c b/refs.c\nindex cb2f825..db8057c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2213,15 +2213,16 @@ int commit_packed_refs(void)\n \tstruct packed_ref_cache *packed_ref_cache =\n \t\tget_packed_ref_cache(&ref_cache);\n \tint error = 0;\n+\tint fd;\n \n \tif (!packed_ref_cache->lock)\n \t\tdie(\"internal error: packed-refs not locked\");\n \twrite_or_die(packed_ref_cache->lock->fd,\n \t\t PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n \n+\tfd = packed_ref_cache->lock->fd;\n \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n-\t\t\t\t 0, write_packed_entry_fn,\n-\t\t\t\t &packed_ref_cache->lock->fd);\n+\t\t\t\t 0, write_packed_entry_fn, &fd);\n \tif (commit_lock_file(packed_ref_cache->lock))\n \t\terror = -1;\n \tpacked_ref_cache->lock = NULL;\n-- \n1.9.1\n"},{"id":"238837","messageId":"1397483695-10888-20-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 19/25] try_merge_strategy(): remove redundant lock_file allocation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:49Z","receivedAt":"2014-04-14T13:54:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"By the time the \"if\" block is entered, the lock_file instance from the\nmain function block is no longer in use, so re-use that one instead of\nallocating a second one.\n\nNote that the \"lock\" variable in the \"if\" block shadowed the \"lock\"\nvariable at function scope, so the only change needed is to remove the\ninner definition.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/merge.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 66d8843..9b7e7cb 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -671,7 +671,6 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n \t\tstruct commit *result;\n-\t\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \t\tint index_fd;\n \t\tstruct commit_list *reversed = NULL;\n \t\tstruct merge_options o;\n-- \n1.9.1\n"},{"id":"238841","messageId":"1397483695-10888-21-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 20/25] try_merge_strategy(): use a statically-allocated lock_file object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:50Z","receivedAt":"2014-04-14T13:54:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Even the one lockfile object needn't be allocated each time the\nfunction is called. Instead, define one statically-allocated\nlock_file object and reuse it for every call.\n\nSuggested-by: Jeff King \nSigned-off-by: Michael Haggerty \n---\n builtin/merge.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9b7e7cb..1efa7b6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -657,16 +657,16 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit_list *remoteheads,\n \t\t\t struct commit *head, const char *head_arg)\n {\n+\tstatic struct lock_file lock;\n \tint index_fd;\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \n-\tindex_fd = hold_locked_index(lock, 1);\n+\tindex_fd = hold_locked_index(&lock, 1);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n \t\t\t(write_cache(index_fd, active_cache, active_nr) ||\n-\t\t\t commit_locked_index(lock)))\n+\t\t\t commit_locked_index(&lock)))\n \t\treturn error(_(\"Unable to write index.\"));\n-\trollback_lock_file(lock);\n+\trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n \t\tint clean, x;\n@@ -699,14 +699,14 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tfor (j = common; j; j = j->next)\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n-\t\tindex_fd = hold_locked_index(lock, 1);\n+\t\tindex_fd = hold_locked_index(&lock, 1);\n \t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n \t\t\t\t(write_cache(index_fd, active_cache, active_nr) ||\n-\t\t\t\t commit_locked_index(lock)))\n+\t\t\t\t commit_locked_index(&lock)))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n-\t\trollback_lock_file(lock);\n+\t\trollback_lock_file(&lock);\n \t\treturn clean ? 0 : 1;\n \t} else {\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n-- \n1.9.1\n"},{"id":"238834","messageId":"1397483695-10888-22-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 21/25] commit_lock_file(): use a strbuf to manage temporary space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:51Z","receivedAt":"2014-04-14T13:54:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Avoid relying on the filename length restrictions that are currently\nchecked by lock_file().\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex fce53f1..0aa2998 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -305,7 +305,8 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar result_file[PATH_MAX];\n+\tstatic struct strbuf result_file = STRBUF_INIT;\n+\tint err;\n \n \tif (lk->fd >= 0 && close_lock_file(lk))\n \t\treturn -1;\n@@ -313,11 +314,12 @@ int commit_lock_file(struct lock_file *lk)\n \tif (!lk->active)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n-\tstrcpy(result_file, lk->filename);\n \t/* remove \".lock\": */\n-\tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n-\n-\tif (rename(lk->filename, result_file))\n+\tstrbuf_add(&result_file, lk->filename,\n+\t\t strlen(lk->filename) - LOCK_SUFFIX_LEN);\n+\terr = rename(lk->filename, result_file.buf);\n+\tstrbuf_reset(&result_file);\n+\tif (err)\n \t\treturn -1;\n \tlk->active = 0;\n \tlk->filename[0] = 0;\n-- \n1.9.1\n"},{"id":"238835","messageId":"1397483695-10888-23-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 22/25] Change lock_file::filename into a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:52Z","receivedAt":"2014-04-14T13:54:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"For now, we still make sure to allocate at least PATH_MAX characters\nfor the strbuf because resolve_symlink() doesn't know how to expand\nthe space for its return value. (That will be fixed in a moment.)\n\nAnother alternative would be to just use a strbuf as scratch space in\nlock_file() but then store a pointer to the naked string in struct\nlock_file. But lock_file objects are often reused. By reusing the\nsame strbuf, we can avoid having to reallocate the string most times\nwhen a lock_file object is reused.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/commit.c | 12 ++++++------\n builtin/reflog.c | 2 +-\n cache.h | 2 +-\n config.c | 6 +++---\n lockfile.c | 45 +++++++++++++++++++--------------------------\n refs.c | 6 +++---\n shallow.c | 6 +++---\n 7 files changed, 36 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8ffb3ef..38c137f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -330,7 +330,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tdie(_(\"unable to create temporary index\"));\n \n \t\told_index_env = getenv(INDEX_ENVIRONMENT);\n-\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename, 1);\n+\t\tsetenv(INDEX_ENVIRONMENT, index_lock.filename.buf, 1);\n \n \t\tif (interactive_add(argc, argv, prefix, patch_interactive) != 0)\n \t\t\tdie(_(\"interactive add failed\"));\n@@ -341,10 +341,10 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n \n \t\tdiscard_cache();\n-\t\tread_cache_from(index_lock.filename);\n+\t\tread_cache_from(index_lock.filename.buf);\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.filename.buf;\n \t}\n \n \t/*\n@@ -368,7 +368,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t close_lock_file(&index_lock))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn index_lock.filename;\n+\t\treturn index_lock.filename.buf;\n \t}\n \n \t/*\n@@ -453,9 +453,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tdie(_(\"unable to write temporary index file\"));\n \n \tdiscard_cache();\n-\tread_cache_from(false_lock.filename);\n+\tread_cache_from(false_lock.filename.buf);\n \n-\treturn false_lock.filename;\n+\treturn false_lock.filename.buf;\n }\n \n static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c12a9784..d7df34e 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -431,7 +431,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n-\t\t\t\tlock->lk->filename);\n+\t\t\t\t\tlock->lk->filename.buf);\n \t\t\tunlink(newlog_path);\n \t\t} else if (rename(newlog_path, log_file)) {\n \t\t\tstatus |= error(\"cannot rename %s to %s\",\ndiff --git a/cache.h b/cache.h\nindex 9019c7d..319251b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -543,7 +543,7 @@ struct lock_file {\n \tvolatile int fd;\n \tvolatile pid_t owner;\n \tchar on_list;\n-\tchar filename[PATH_MAX];\n+\tstruct strbuf filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/config.c b/config.c\nindex 6821cef..78c3dad 100644\n--- a/config.c\n+++ b/config.c\n@@ -1706,7 +1706,7 @@ out_free:\n \treturn ret;\n \n write_err_out:\n-\tret = write_error(lock->filename);\n+\tret = write_error(lock->filename.buf);\n \tgoto out_free;\n \n }\n@@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\t\t}\n \t\t\t\tstore.baselen = strlen(new_name);\n \t\t\t\tif (!store_write_section(out_fd, new_name)) {\n-\t\t\t\t\tret = write_error(lock->filename);\n+\t\t\t\t\tret = write_error(lock->filename.buf);\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t\t/*\n@@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\t\tcontinue;\n \t\tlength = strlen(output);\n \t\tif (write_in_full(out_fd, output, length) != length) {\n-\t\t\tret = write_error(lock->filename);\n+\t\t\tret = write_error(lock->filename.buf);\n \t\t\tgoto out;\n \t\t}\n \t}\ndiff --git a/lockfile.c b/lockfile.c\nindex 0aa2998..f552e33 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -184,13 +184,6 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\t/*\n-\t * subtract LOCK_SUFFIX_LEN from size to make sure there's\n-\t * room for adding \".lock\" for the lock file name:\n-\t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) -\n-\t\t\t\t\t LOCK_SUFFIX_LEN;\n-\n \tif (!lock_file_list) {\n \t\t/* One-time initialization */\n \t\tsigchain_push_common(remove_lock_file_on_signal);\n@@ -205,26 +198,26 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_init(&lk->filename, PATH_MAX);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t}\n \n-\tif (strlen(path) >= max_path_len)\n-\t\treturn -1;\n-\tstrcpy(lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n-\tstrcat(lk->filename, LOCK_SUFFIX);\n-\tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n+\tstrbuf_addstr(&lk->filename, path);\n+\tif (!(flags & LOCK_NODEREF)) {\n+\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n+\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\n+\t}\n+\tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n+\tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n \tlk->active = 1;\n-\tif (adjust_shared_perm(lk->filename)) {\n-\t\terror(\"cannot fix permission bits on %s\", lk->filename);\n+\tif (adjust_shared_perm(lk->filename.buf)) {\n+\t\terror(\"cannot fix permission bits on %s\", lk->filename.buf);\n \t\trollback_lock_file(lk);\n \t\treturn -1;\n \t}\n@@ -315,14 +308,14 @@ int commit_lock_file(struct lock_file *lk)\n \t\tdie(\"BUG: attempt to commit unlocked object\");\n \n \t/* remove \".lock\": */\n-\tstrbuf_add(&result_file, lk->filename,\n-\t\t strlen(lk->filename) - LOCK_SUFFIX_LEN);\n-\terr = rename(lk->filename, result_file.buf);\n+\tstrbuf_add(&result_file, lk->filename.buf,\n+\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n+\terr = rename(lk->filename.buf, result_file.buf);\n \tstrbuf_reset(&result_file);\n \tif (err)\n \t\treturn -1;\n \tlk->active = 0;\n-\tlk->filename[0] = 0;\n+\tstrbuf_reset(&lk->filename);\n \treturn 0;\n }\n \n@@ -344,10 +337,10 @@ int commit_locked_index(struct lock_file *lk)\n \tif (alternate_index_output) {\n \t\tif (lk->fd >= 0 && close_lock_file(lk))\n \t\t\treturn -1;\n-\t\tif (rename(lk->filename, alternate_index_output))\n+\t\tif (rename(lk->filename.buf, alternate_index_output))\n \t\t\treturn -1;\n \t\tlk->active = 0;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t\treturn 0;\n \t}\n \telse\n@@ -359,8 +352,8 @@ void rollback_lock_file(struct lock_file *lk)\n \tif (lk->active) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose_lock_file(lk);\n-\t\tunlink_or_warn(lk->filename);\n+\t\tunlink_or_warn(lk->filename.buf);\n \t\tlk->active = 0;\n-\t\tlk->filename[0] = 0;\n+\t\tstrbuf_reset(&lk->filename);\n \t}\n }\ndiff --git a/refs.c b/refs.c\nindex db8057c..4bad084 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2492,8 +2492,8 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n \t\tchar *loose_filename = xmemdupz(\n-\t\t\t\tlock->lk->filename,\n-\t\t\t\tstrlen(lock->lk->filename) - LOCK_SUFFIX_LEN);\n+\t\t\t\tlock->lk->filename.buf,\n+\t\t\t\tlock->lk->filename.len - LOCK_SUFFIX_LEN);\n \t\tint err = unlink_or_warn(loose_filename);\n \t\tfree(loose_filename);\n \t\tif (err && errno != ENOENT)\n@@ -2831,7 +2831,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \tif (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||\n \t write_in_full(lock->lock_fd, &term, 1) != 1\n \t\t|| close_ref(lock) < 0) {\n-\t\terror(\"Couldn't write %s\", lock->lk->filename);\n+\t\terror(\"Couldn't write %s\", lock->lk->filename.buf);\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\ndiff --git a/shallow.c b/shallow.c\nindex 0b267b6..cb29a66 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -269,8 +269,8 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock->filename);\n-\t\t*alternate_shallow_file = shallow_lock->filename;\n+\t\t\t\t shallow_lock->filename.buf);\n+\t\t*alternate_shallow_file = shallow_lock->filename.buf;\n \t} else\n \t\t/*\n \t\t * is_repository_shallow() sees empty string as \"no\n@@ -316,7 +316,7 @@ void prune_shallow(int show_only)\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n \t\t\tdie_errno(\"failed to write to %s\",\n-\t\t\t\t shallow_lock.filename);\n+\t\t\t\t shallow_lock.filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n \t\tunlink(git_path(\"shallow\"));\n-- \n1.9.1\n"},{"id":"238832","messageId":"1397483695-10888-24-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 23/25] resolve_symlink(): use a strbuf for internal scratch space","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:53Z","receivedAt":"2014-04-14T13:54:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from shortening and simplifying the code, this removes another\nplace where the path name length is arbitrarily limited.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 33 ++++++++++++---------------------\n 1 file changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex f552e33..59ed758 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -141,44 +141,35 @@ static char *last_path_elm(char *p)\n static char *resolve_symlink(char *p, size_t s)\n {\n \tint depth = MAXDEPTH;\n+\tstatic struct strbuf link = STRBUF_INIT;\n \n \twhile (depth--) {\n-\t\tchar link[PATH_MAX];\n-\t\tint link_len = readlink(p, link, sizeof(link));\n-\t\tif (link_len < 0) {\n-\t\t\t/* not a symlink anymore */\n-\t\t\treturn p;\n-\t\t}\n-\t\telse if (link_len < sizeof(link))\n-\t\t\t/* readlink() never null-terminates */\n-\t\t\tlink[link_len] = '\\0';\n-\t\telse {\n-\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\treturn p;\n-\t\t}\n+\t\tif (strbuf_readlink(&link, p, strlen(p)) < 0)\n+\t\t\tbreak;\n \n-\t\tif (is_absolute_path(link)) {\n+\t\tif (is_absolute_path(link.buf)) {\n \t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link_len < s)\n-\t\t\t\tstrcpy(p, link);\n+\t\t\tif (link.len < s)\n+\t\t\t\tstrcpy(p, link.buf);\n \t\t\telse {\n \t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n-\t\t\t * link is a relative path, so I must replace the\n+\t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n \t\t\tchar *r = (char *)last_path_elm(p);\n-\t\t\tif (r - p + link_len < s)\n-\t\t\t\tstrcpy(r, link);\n+\t\t\tif (r - p + link.len < s)\n+\t\t\t\tstrcpy(r, link.buf);\n \t\t\telse {\n \t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \t}\n+\tstrbuf_reset(&link);\n \treturn p;\n }\n \n-- \n1.9.1\n"},{"id":"238833","messageId":"1397483695-10888-25-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 24/25] resolve_symlink(): take a strbuf parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:54Z","receivedAt":"2014-04-14T13:54:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change resolve_symlink() to take a strbuf rather than a string as\nparameter. This simplifies the code and removes an arbitrary pathname\nlength restriction. It also means that lock_file's filename field no\nlonger needs to be initialized to a large size.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 53 +++++++++++++++++++----------------------------------\n 1 file changed, 19 insertions(+), 34 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 59ed758..65f56ef 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -124,53 +124,40 @@ static char *last_path_elm(char *p)\n #define MAXDEPTH 5\n \n /*\n- * p = path that may be a symlink\n- * s = full size of p\n+ * path contains a path that might be a symlink.\n *\n- * If p is a symlink, attempt to overwrite p with a path to the real\n- * file or directory (which may or may not exist), following a chain of\n- * symlinks if necessary. Otherwise, leave p unmodified.\n+ * If path is a symlink, attempt to overwrite it with a path to the\n+ * real file or directory (which may or may not exist), following a\n+ * chain of symlinks if necessary. Otherwise, leave path unmodified.\n *\n- * This is a best-effort routine. If an error occurs, p will either be\n- * left unmodified or will name a different symlink in a symlink chain\n- * that started with p's initial contents.\n- *\n- * Always returns p.\n+ * This is a best-effort routine. If an error occurs, path will\n+ * either be left unmodified or will name a different symlink in a\n+ * symlink chain that started with the original path.\n */\n-\n-static char *resolve_symlink(char *p, size_t s)\n+static void resolve_symlink(struct strbuf *path)\n {\n \tint depth = MAXDEPTH;\n \tstatic struct strbuf link = STRBUF_INIT;\n \n \twhile (depth--) {\n-\t\tif (strbuf_readlink(&link, p, strlen(p)) < 0)\n+\t\tif (strbuf_readlink(&link, path->buf, path->len) < 0)\n \t\t\tbreak;\n \n-\t\tif (is_absolute_path(link.buf)) {\n+\t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link.len < s)\n-\t\t\t\tstrcpy(p, link.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t} else {\n+\t\t\tstrbuf_reset(path);\n+\t\telse {\n \t\t\t/*\n \t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = (char *)last_path_elm(p);\n-\t\t\tif (r - p + link.len < s)\n-\t\t\t\tstrcpy(r, link.buf);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\t\tchar *r = last_path_elm(path->buf);\n+\t\t\tstrbuf_setlen(path, r - path->buf);\n \t\t}\n+\n+\t\tstrbuf_addbuf(path, &link);\n \t}\n \tstrbuf_reset(&link);\n-\treturn p;\n }\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n@@ -189,16 +176,14 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\tlk->active = 0;\n \t\tlk->owner = 0;\n \t\tlk->on_list = 1;\n-\t\tstrbuf_init(&lk->filename, PATH_MAX);\n+\t\tstrbuf_init(&lk->filename, 0);\n \t\tlk->next = lock_file_list;\n \t\tlock_file_list = lk;\n \t}\n \n \tstrbuf_addstr(&lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF)) {\n-\t\tresolve_symlink(lk->filename.buf, lk->filename.alloc);\n-\t\tstrbuf_setlen(&lk->filename, strlen(lk->filename.buf));\n-\t}\n+\tif (!(flags & LOCK_NODEREF))\n+\t\tresolve_symlink(&lk->filename);\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n-- \n1.9.1\n"},{"id":"238836","messageId":"1397483695-10888-26-git-send-email-mhagger@alum.mit.edu","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 25/25] trim_last_path_elm(): replace last_path_elm()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-14T13:54:55Z","receivedAt":"2014-04-14T13:54:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rewrite last_path_elm() to take a strbuf parameter and to trim off the\nlast path name element in place rather than returning a pointer to the\nbeginning of the last path name element. This simplifies the function\na bit and makes it integrate better with its caller, which is also\nstrbuf-based. Rename the function accordingly.\n\nSigned-off-by: Michael Haggerty \n---\n lockfile.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 65f56ef..2515577 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -91,32 +91,28 @@ static void remove_lock_file_on_signal(int signo)\n }\n \n /*\n- * p = absolute or relative path name\n+ * path = absolute or relative path name\n *\n- * Return a pointer into p showing the beginning of the last path name\n- * element. If p is empty or the root directory (\"/\"), just return p.\n+ * Remove the last path name element from path (leaving the preceding\n+ * \"/\", if any). If path is empty or the root directory (\"/\"), set\n+ * path to the empty string.\n */\n-static char *last_path_elm(char *p)\n+static void trim_last_path_elm(struct strbuf *path)\n {\n-\t/* r starts pointing to null at the end of the string */\n-\tchar *r = strchr(p, '\\0');\n-\n-\tif (r == p)\n-\t\treturn p; /* just return empty string */\n-\n-\tr--; /* back up to last non-null character */\n+\tint i = path->len;\n \n \t/* back up past trailing slashes, if any */\n-\twhile (r > p && *r == '/')\n-\t\tr--;\n+\twhile (i && path->buf[i - 1] == '/')\n+\t\ti--;\n \n \t/*\n-\t * then go backwards until I hit a slash, or the beginning of\n-\t * the string\n+\t * then go backwards until a slash, or the beginning of the\n+\t * string\n \t */\n-\twhile (r > p && *(r-1) != '/')\n-\t\tr--;\n-\treturn r;\n+\twhile (i && path->buf[i - 1] != '/')\n+\t\ti--;\n+\n+\tstrbuf_setlen(path, i);\n }\n \n \n@@ -146,14 +142,12 @@ static void resolve_symlink(struct strbuf *path)\n \t\tif (is_absolute_path(link.buf))\n \t\t\t/* absolute path simply replaces p */\n \t\t\tstrbuf_reset(path);\n-\t\telse {\n+\t\telse\n \t\t\t/*\n \t\t\t * link is a relative path, so replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = last_path_elm(path->buf);\n-\t\t\tstrbuf_setlen(path, r - path->buf);\n-\t\t}\n+\t\t\ttrim_last_path_elm(path);\n \n \t\tstrbuf_addbuf(path, &link);\n \t}\n-- \n1.9.1\n"},{"id":"238877","messageId":"534CD695.20306@viscovery.net","threadId":"36409","inReplyTo":"1397483695-10888-17-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 16/25] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-04-15T06:49:57Z","receivedAt":"2014-04-15T06:49:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/14/2014 15:54, schrieb Michael Haggerty:\n> diff --git a/lockfile.c b/lockfile.c\n> index 664b0c3..1453a7a 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -292,6 +292,9 @@ int commit_lock_file(struct lock_file *lk)\n> \tif (lk->fd >= 0 && close_lock_file(lk))\n> \t\treturn -1;\n> \n> +\tif (!lk->filename[0])\n> +\t\tdie(\"BUG: attempt to commit unlocked object\");\n> +\n\nShouldn't this be the first thing to check after entering the function?\n\n> \tstrcpy(result_file, lk->filename);\n> \t/* remove \".lock\": */\n> \tresult_file[strlen(result_file) - LOCK_SUFFIX_LEN] = 0;\n> \n"},{"id":"238878","messageId":"534CD7D2.4060005@viscovery.net","threadId":"36409","inReplyTo":"1397483695-10888-19-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2014-04-15T06:55:14Z","receivedAt":"2014-04-15T06:55:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/14/2014 15:54, schrieb Michael Haggerty:\n> The function remove_lock_file_on_signal() is used as a signal handler.\n> It is not realistic to make the signal handler conform strictly to the\n> C standard, which is very restrictive about what a signal handler is\n> allowed to do. But let's increase the likelihood that it will work:\n> \n> The lock_file_list global variable and several fields from struct\n> lock_file are used by the signal handler. Declare those values\n> \"volatile\" to increase the chance that the signal handler will see a\n> valid object state.\n\nYes, it's important that the signal handler sees a valid object state, and\n\"volatile\" can help here. But I think the reason why it helps is not\nobvious, and it should be mentioned in the commit message:\n\nIt is not so much that \"volatile\" forces the compiler to lay down each\naccess of the variable coded in C in the assembly code, but more\nimportantly, that \"volatile\" disallows any re-ordering of these accesses.\nThen:\n\n- 'lock->active = 1' must be the last assignment during setup\n\n- 'lock->active = 0' must be the first assignment during tear-down.\n\n- Ideally, all members of struct lock_file should be \"volatile\".\n\nThe last point is important because the compiler is allowed to re-order\naccesses to non-\"volatile\" variables across \"volatile\" accesses. I say\n\"ideally\" because if filename were defined \"volatile filename[PATH_MAX]\",\nstrcpy() could not be used anymore. OTOH, it is unlikely that a compiler\nre-orders a strcpy() call across other expressions, and we can get away\nwithout \"volatile\" in the \"filename\" case in practice.\n\n> Suggested-by: Johannes Sixt \n\nNot a big deal, but just in case you re-roll again and you do not forget:\n\n Johannes Sixt \n\nis preferred.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> cache.h | 6 +++---\n> lockfile.c | 2 +-\n> refs.c | 5 +++--\n> 3 files changed, 7 insertions(+), 6 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index b7af173..9019c7d 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -538,10 +538,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n> #define LOCK_SUFFIX_LEN 5\n> \n> struct lock_file {\n> -\tstruct lock_file *next;\n> +\tstruct lock_file *volatile next;\n> \tvolatile sig_atomic_t active;\n> -\tint fd;\n> -\tpid_t owner;\n> +\tvolatile int fd;\n> +\tvolatile pid_t owner;\n> \tchar on_list;\n> \tchar filename[PATH_MAX];\n> };\n> diff --git a/lockfile.c b/lockfile.c\n> index 50a0541..fce53f1 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -69,7 +69,7 @@\n> * See Documentation/api-lockfile.txt for more information.\n> */\n> \n> -static struct lock_file *lock_file_list;\n> +static struct lock_file *volatile lock_file_list;\n> static const char *alternate_index_output;\n> \n> static void remove_lock_file(void)\n> diff --git a/refs.c b/refs.c\n> index cb2f825..db8057c 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2213,15 +2213,16 @@ int commit_packed_refs(void)\n> \tstruct packed_ref_cache *packed_ref_cache =\n> \t\tget_packed_ref_cache(&ref_cache);\n> \tint error = 0;\n> +\tint fd;\n> \n> \tif (!packed_ref_cache->lock)\n> \t\tdie(\"internal error: packed-refs not locked\");\n> \twrite_or_die(packed_ref_cache->lock->fd,\n> \t\t PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n> \n> +\tfd = packed_ref_cache->lock->fd;\n> \tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n> -\t\t\t\t 0, write_packed_entry_fn,\n> -\t\t\t\t &packed_ref_cache->lock->fd);\n> +\t\t\t\t 0, write_packed_entry_fn, &fd);\n> \tif (commit_lock_file(packed_ref_cache->lock))\n> \t\terror = -1;\n> \tpacked_ref_cache->lock = NULL;\n> \n"},{"id":"238895","messageId":"534D7D2E.7060100@web.de","threadId":"36409","inReplyTo":"1397483695-10888-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/25] Lockfile correctness and refactoring","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-04-15T18:40:46Z","receivedAt":"2014-04-15T18:40:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"refs.c:\nint close_ref(struct ref_lock *lock)\n{\n\tif (close_lock_file(lock->lk))\n\t\treturn -1;\n\tlock->lock_fd = -1;\n\treturn 0;\n}\n\nWhen the close() fails, fd is still >= 0, even if the file is closed.\n\nCould it be written like this ?\n\nint close_ref(struct ref_lock *lock)\n{\n\treturn close_lock_file(lock->lk);\n}\n=====================\nlockfile.c, line 49\n * - filename holds the filename of the lockfile\n\nI think we have a strbuf here? (which is a good thing),\nshould we write:\n * - strbuf filename holds the filename of the lockfile\n----------\n(and at some places filename[0] is mentioned,\nshould that be filename.buf[0] ?)\n\n\n\n=========================\nint commit_lock_file(struct lock_file *lk)\n{\n\tstatic struct strbuf result_file = STRBUF_INIT;\n\tint err;\n\n\tif (lk->fd >= 0 && close_lock_file(lk))\n\t\treturn -1;\n##What happens if close() fails and close_lock_file() returns != 0?\n##Is the lk now in a good shaped state?\nI think the file which had been open by lk->fd is in an unkown state,\nand should not be used for anything.\nWhen I remember it right, an error on close() can mean \"the file could not\nbe written and closed as expected, it can be truncated or corrupted.\nThis can happen on a network file system like NFS, or probably even other FS.\nFor me the failing of close() means I smell a need for a rollback.\n\n\tif (!lk->active)\n\t\tdie(\"BUG: attempt to commit unlocked object\");\n\n\t/* remove \".lock\": */\n\tstrbuf_add(&result_file, lk->filename.buf,\n\t\t lk->filename.len - LOCK_SUFFIX_LEN);\n\terr = rename(lk->filename.buf, result_file.buf);\n\tstrbuf_reset(&result_file);\n\tif (err)\n\t\treturn -1;\n\tlk->active = 0;\n\tstrbuf_reset(&lk->filename);\n\treturn 0;\n}\n\n================\nPlease treat my comments more than questions rather than answers,\nthanks for an interesting reading\n"},{"id":"238936","messageId":"534E9F06.6010300@alum.mit.edu","threadId":"36409","inReplyTo":"534CD695.20306@viscovery.net","subject":"Re: [PATCH v3 16/25] commit_lock_file(): die() if called for unlocked lockfile object","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T15:17:26Z","receivedAt":"2014-04-16T15:17:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2014 08:49 AM, Johannes Sixt wrote:\n> Am 4/14/2014 15:54, schrieb Michael Haggerty:\n>> diff --git a/lockfile.c b/lockfile.c\n>> index 664b0c3..1453a7a 100644\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -292,6 +292,9 @@ int commit_lock_file(struct lock_file *lk)\n>> \tif (lk->fd >= 0 && close_lock_file(lk))\n>> \t\treturn -1;\n>> \n>> +\tif (!lk->filename[0])\n>> +\t\tdie(\"BUG: attempt to commit unlocked object\");\n>> +\n> \n> Shouldn't this be the first thing to check after entering the function?\n\nYou're right; I will change it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238938","messageId":"534EA36E.1030208@alum.mit.edu","threadId":"36409","inReplyTo":"534CD7D2.4060005@viscovery.net","subject":"Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T15:36:14Z","receivedAt":"2014-04-16T15:36:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2014 08:55 AM, Johannes Sixt wrote:\n> Am 4/14/2014 15:54, schrieb Michael Haggerty:\n>> The function remove_lock_file_on_signal() is used as a signal handler.\n>> It is not realistic to make the signal handler conform strictly to the\n>> C standard, which is very restrictive about what a signal handler is\n>> allowed to do. But let's increase the likelihood that it will work:\n>>\n>> The lock_file_list global variable and several fields from struct\n>> lock_file are used by the signal handler. Declare those values\n>> \"volatile\" to increase the chance that the signal handler will see a\n>> valid object state.\n> \n> Yes, it's important that the signal handler sees a valid object state, and\n> \"volatile\" can help here. But I think the reason why it helps is not\n> obvious, and it should be mentioned in the commit message:\n> \n> It is not so much that \"volatile\" forces the compiler to lay down each\n> access of the variable coded in C in the assembly code, but more\n> importantly, that \"volatile\" disallows any re-ordering of these accesses.\n> Then:\n> \n> - 'lock->active = 1' must be the last assignment during setup\n> \n> - 'lock->active = 0' must be the first assignment during tear-down.\n> \n> - Ideally, all members of struct lock_file should be \"volatile\".\n> \n> The last point is important because the compiler is allowed to re-order\n> accesses to non-\"volatile\" variables across \"volatile\" accesses. I say\n> \"ideally\" because if filename were defined \"volatile filename[PATH_MAX]\",\n> strcpy() could not be used anymore. OTOH, it is unlikely that a compiler\n> re-orders a strcpy() call across other expressions, and we can get away\n> without \"volatile\" in the \"filename\" case in practice.\n\nThanks for the clarification. I will edit the commit message to better\nexplain the rationale.\n\n>> Suggested-by: Johannes Sixt \n> \n> Not a big deal, but just in case you re-roll again and you do not forget:\n> \n> Johannes Sixt \n> \n> is preferred.\n\nACK.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238979","messageId":"534EDF20.70201@alum.mit.edu","threadId":"36409","inReplyTo":"534D7D2E.7060100@web.de","subject":"Re: [PATCH v3 00/25] Lockfile correctness and refactoring","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T19:50:56Z","receivedAt":"2014-04-16T19:50:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/15/2014 08:40 PM, Torsten Bögershausen wrote:\n> refs.c:\n> int close_ref(struct ref_lock *lock)\n> {\n> \tif (close_lock_file(lock->lk))\n> \t\treturn -1;\n> \tlock->lock_fd = -1;\n> \treturn 0;\n> }\n> \n> When the close() fails, fd is still >= 0, even if the file is closed.\n> \n> Could it be written like this ?\n> \n> int close_ref(struct ref_lock *lock)\n> {\n> \treturn close_lock_file(lock->lk);\n> }\n\nIt seem to me it would be better to set lock->lock_fd to -1 regardless\nof whether close_lock_file() succeeds. Or maybe this field is not even\nneeded, and instead lock->lk->fd should be used.\n\nThis is a bit beyond the scope of this patch series, but I will put it\non my todo list.\n\n> =====================\n> lockfile.c, line 49\n> * - filename holds the filename of the lockfile\n> \n> I think we have a strbuf here? (which is a good thing),\n> should we write:\n> * - strbuf filename holds the filename of the lockfile\n> ----------\n> (and at some places filename[0] is mentioned,\n> should that be filename.buf[0] ?)\n\nI think it is OK to speak of a strbuf as \"holding\" a filename (what else\nwould that construct mean?\n\nBut you are correct that the comments shouldn't speak of filename[0]\nanymore. I will fix it.\n\n> =========================\n> int commit_lock_file(struct lock_file *lk)\n> {\n> \tstatic struct strbuf result_file = STRBUF_INIT;\n> \tint err;\n> \n> \tif (lk->fd >= 0 && close_lock_file(lk))\n> \t\treturn -1;\n> ##What happens if close() fails and close_lock_file() returns != 0?\n> ##Is the lk now in a good shaped state?\n> I think the file which had been open by lk->fd is in an unkown state,\n> and should not be used for anything.\n> When I remember it right, an error on close() can mean \"the file could not\n> be written and closed as expected, it can be truncated or corrupted.\n> This can happen on a network file system like NFS, or probably even other FS.\n> For me the failing of close() means I smell a need for a rollback.\n\nYes, this is a good catch. I think a rollback should definitely be done\nin this case. I will fix it.\n\nIn fact, I'm wondering whether it would be appropriate for\nclose_lock_file() itself to do a rollback if close() fails. I guess I\nwill first have to audit callers to make sure that they don't try to use\nlock_file::filename after a failed close_lock_file() (e.g., for\ngenerating an error message).\n\n> Please treat my comments more than questions rather than answers,\n> thanks for an interesting reading\n\nThanks for your helpful comments!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}