From e197c21807dacadc8305250baa0b9228819189d4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:05 +0200 Subject: 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 --- cache.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cache.h') 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 *); -- cgit v1.2.1 From 7108ad232fc7a4c889e82b40c52125adc9796ff5 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:15 +0200 Subject: 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 ++++ 1 file changed, 4 insertions(+) (limited to 'cache.h') 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; -- cgit v1.2.1 From 707103fdfd0c03511fa547d9b80638d8160f1a88 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:27 +0200 Subject: 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 + 1 file changed, 1 insertion(+) (limited to 'cache.h') 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; -- cgit v1.2.1 From 2091c5062c6fd928e1ad3e7c059243e597cb8bbf Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:28 +0200 Subject: 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 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'cache.h') 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]; }; -- cgit v1.2.1 From cf6950d3bfe1447ac04867b1f5654a2fc9c5db96 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:32 +0200 Subject: 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 --- cache.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cache.h') 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 -- cgit v1.2.1 From 751bacedaa507b7b6d10b2c1f48e019a01a8fa6e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:36 +0200 Subject: 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 --- cache.h | 1 + 1 file changed, 1 insertion(+) (limited to 'cache.h') 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 *); -- cgit v1.2.1 From 47ba4662bfd755829edd24428cf2e4bc492d70a6 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:37 +0200 Subject: 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 --- cache.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'cache.h') 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); -- cgit v1.2.1 From ec38b4e482e96e62762452cab5714e55abdb48c3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:39 +0200 Subject: 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 --- cache.h | 1 + 1 file changed, 1 insertion(+) (limited to 'cache.h') 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 *); -- cgit v1.2.1 From 697cc8efd944a32ca472337cd6640004c474b788 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:42 +0200 Subject: 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 --- cache.h | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) (limited to 'cache.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 */ -- cgit v1.2.1