diff mbox series

[19/23] fsm-health-win32: force shutdown daemon if worktree root moves

Message ID 023fcd6e2b1163ab3d23b0d5933c14586d814ce0.1644940774.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Builtin FSMonitor Part 3 | expand

Commit Message

Jeff Hostetler Feb. 15, 2022, 3:59 p.m. UTC
From: Jeff Hostetler <jeffhost@microsoft.com>

Force shutdown fsmonitor daemon if the worktree root directory
is moved, renamed, or deleted.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/fsmonitor/fsm-health-win32.c | 133 ++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

Comments

Derrick Stolee Feb. 24, 2022, 4:09 p.m. UTC | #1
On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> diff --git a/compat/fsmonitor/fsm-health-win32.c b/compat/fsmonitor/fsm-health-win32.c
> index 3c3453369cd..2526ad9194f 100644
> --- a/compat/fsmonitor/fsm-health-win32.c
> +++ b/compat/fsmonitor/fsm-health-win32.c
> @@ -14,7 +14,10 @@ enum interval_fn_ctx { CTX_INIT = 0, CTX_TERM, CTX_TIMER };
>  typedef int (interval_fn)(struct fsmonitor_daemon_state *state,
>  			  enum interval_fn_ctx ctx);
>  
> +static interval_fn has_worktree_moved;
> +
>  static interval_fn *table[] = {
> +	has_worktree_moved,
>  	NULL, /* must be last */
>  };

Looking at this now, I think table[] should be defined immediately
before fsm_health__loop() so it is easier to see how they interact.
It also avoids this static declaration of the function before its
implementation.

Or, is there a reason it is so high up in the file?

Thanks,
-Stolee
Jeff Hostetler March 3, 2022, 6 p.m. UTC | #2
On 2/24/22 11:09 AM, Derrick Stolee wrote:
> On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> diff --git a/compat/fsmonitor/fsm-health-win32.c b/compat/fsmonitor/fsm-health-win32.c
>> index 3c3453369cd..2526ad9194f 100644
>> --- a/compat/fsmonitor/fsm-health-win32.c
>> +++ b/compat/fsmonitor/fsm-health-win32.c
>> @@ -14,7 +14,10 @@ enum interval_fn_ctx { CTX_INIT = 0, CTX_TERM, CTX_TIMER };
>>   typedef int (interval_fn)(struct fsmonitor_daemon_state *state,
>>   			  enum interval_fn_ctx ctx);
>>   
>> +static interval_fn has_worktree_moved;
>> +
>>   static interval_fn *table[] = {
>> +	has_worktree_moved,
>>   	NULL, /* must be last */
>>   };
> 
> Looking at this now, I think table[] should be defined immediately
> before fsm_health__loop() so it is easier to see how they interact.
> It also avoids this static declaration of the function before its
> implementation.
> 
> Or, is there a reason it is so high up in the file?

I don't think so.  I was trying to keep all of the public API
routines at the bottom, but all of the static stuff is pretty
much free to move around.

I'll revisit.

Thanks
Jeff
diff mbox series

Patch

diff --git a/compat/fsmonitor/fsm-health-win32.c b/compat/fsmonitor/fsm-health-win32.c
index 3c3453369cd..2526ad9194f 100644
--- a/compat/fsmonitor/fsm-health-win32.c
+++ b/compat/fsmonitor/fsm-health-win32.c
@@ -14,7 +14,10 @@  enum interval_fn_ctx { CTX_INIT = 0, CTX_TERM, CTX_TIMER };
 typedef int (interval_fn)(struct fsmonitor_daemon_state *state,
 			  enum interval_fn_ctx ctx);
 
+static interval_fn has_worktree_moved;
+
 static interval_fn *table[] = {
+	has_worktree_moved,
 	NULL, /* must be last */
 };
 
@@ -45,6 +48,12 @@  struct fsm_health_data
 	HANDLE hHandles[1]; /* the array does not own these handles */
 #define HEALTH_SHUTDOWN 0
 	int nr_handles; /* number of active event handles */
+
+	struct wt_moved
+	{
+		wchar_t wpath[MAX_PATH + 1];
+		BY_HANDLE_FILE_INFORMATION bhfi;
+	} wt_moved;
 };
 
 int fsm_health__ctor(struct fsmonitor_daemon_state *state)
@@ -76,6 +85,130 @@  void fsm_health__dtor(struct fsmonitor_daemon_state *state)
 	FREE_AND_NULL(state->health_data);
 }
 
+static int lookup_bhfi(wchar_t *wpath,
+		       BY_HANDLE_FILE_INFORMATION *bhfi)
+{
+	DWORD desired_access = FILE_LIST_DIRECTORY;
+	DWORD share_mode =
+		FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE;
+	HANDLE hDir;
+
+	hDir = CreateFileW(wpath, desired_access, share_mode, NULL,
+			   OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
+	if (hDir == INVALID_HANDLE_VALUE) {
+		error(_("[GLE %ld] health thread could not open '%ls'"),
+		      GetLastError(), wpath);
+		return -1;
+	}
+
+	if (!GetFileInformationByHandle(hDir, bhfi)) {
+		error(_("[GLE %ld] health thread getting BHFI for '%ls'"),
+		      GetLastError(), wpath);
+		CloseHandle(hDir);
+		return -1;
+	}
+
+	CloseHandle(hDir);
+	return 0;
+}
+
+static int bhfi_eq(const BY_HANDLE_FILE_INFORMATION *bhfi_1,
+		   const BY_HANDLE_FILE_INFORMATION *bhfi_2)
+{
+	return (bhfi_1->dwVolumeSerialNumber == bhfi_2->dwVolumeSerialNumber &&
+		bhfi_1->nFileIndexHigh == bhfi_2->nFileIndexHigh &&
+		bhfi_1->nFileIndexLow == bhfi_2->nFileIndexLow);
+}
+
+/*
+ * Shutdown if the original worktree root directory been deleted,
+ * moved, or renamed?
+ *
+ * Since the main thread did a "chdir(getenv($HOME))" and our CWD
+ * is not in the worktree root directory and because the listener
+ * thread added FILE_SHARE_DELETE to the watch handle, it is possible
+ * for the root directory to be moved or deleted while we are still
+ * watching it.  We want to detect that here and force a shutdown.
+ *
+ * Granted, a delete MAY cause some operations to fail, such as
+ * GetOverlappedResult(), but it is not guaranteed.  And because
+ * ReadDirectoryChangesW() only reports on changes *WITHIN* the
+ * directory, not changes *ON* the directory, our watch will not
+ * receive a delete event for it.
+ *
+ * A move/rename of the worktree root will also not generate an event.
+ * And since the listener thread already has an open handle, it may
+ * continue to receive events for events within the directory.
+ * However, the pathname of the named-pipe was constructed using the
+ * original location of the worktree root.  (Remember named-pipes are
+ * stored in the NPFS and not in the actual file system.)  Clients
+ * trying to talk to the worktree after the move/rename will not
+ * reach our daemon process, since we're still listening on the
+ * pipe with original path.
+ *
+ * Furthermore, if the user does something like:
+ *
+ *   $ mv repo repo.old
+ *   $ git init repo
+ *
+ * A new daemon cannot be started in the new instance of "repo"
+ * because the named-pipe is still being used by the daemon on
+ * the original instance.
+ *
+ * So, detect move/rename/delete and shutdown.  This should also
+ * handle unsafe drive removal.
+ *
+ * We use the file system unique ID to distinguish the original
+ * directory instance from a new instance and force a shutdown
+ * if the unique ID changes.
+ *
+ * Since a worktree move/rename/delete/unmount doesn't happen
+ * that often (and we can't get an immediate event anyway), we
+ * use a timeout and periodically poll it.
+ */
+static int has_worktree_moved(struct fsmonitor_daemon_state *state,
+			      enum interval_fn_ctx ctx)
+{
+	struct fsm_health_data *data = state->health_data;
+	BY_HANDLE_FILE_INFORMATION bhfi;
+	int r;
+
+	switch (ctx) {
+	case CTX_TERM:
+		return 0;
+
+	case CTX_INIT:
+		if (xutftowcs_path(data->wt_moved.wpath,
+				   state->path_worktree_watch.buf) < 0) {
+			error(_("could not convert to wide characters: '%s'"),
+			      state->path_worktree_watch.buf);
+			return -1;
+		}
+
+		/*
+		 * On the first call we lookup the unique sequence ID for
+		 * the worktree root directory.
+		 */
+		return lookup_bhfi(data->wt_moved.wpath, &data->wt_moved.bhfi);
+
+	case CTX_TIMER:
+		r = lookup_bhfi(data->wt_moved.wpath, &bhfi);
+		if (r)
+			return r;
+		if (!bhfi_eq(&data->wt_moved.bhfi, &bhfi)) {
+			error(_("BHFI changed '%ls'"), data->wt_moved.wpath);
+			return -1;
+		}
+		return 0;
+
+	default:
+		die("unhandled case in 'has_worktree_moved': %d",
+		    (int)ctx);
+	}
+
+	return 0;
+}
+
 void fsm_health__loop(struct fsmonitor_daemon_state *state)
 {
 	struct fsm_health_data *data = state->health_data;