diff mbox series

[v2,04/28] fsmonitor-ipc: create client routines for git-fsmonitor--daemon

Message ID e4a263728773381a64b8662c0577a3f12eff4ca0.1621691828.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Builtin FSMonitor Feature | expand

Commit Message

Jeff Hostetler May 22, 2021, 1:56 p.m. UTC
From: Jeff Hostetler <jeffhost@microsoft.com>

Create fsmonitor_ipc__*() client routines to spawn the built-in file
system monitor daemon and send it an IPC request using the `Simple
IPC` API.

Stub in empty fsmonitor_ipc__*() functions for unsupported platforms.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile        |   1 +
 fsmonitor-ipc.c | 179 ++++++++++++++++++++++++++++++++++++++++++++++++
 fsmonitor-ipc.h |  48 +++++++++++++
 3 files changed, 228 insertions(+)
 create mode 100644 fsmonitor-ipc.c
 create mode 100644 fsmonitor-ipc.h

Comments

Johannes Schindelin June 2, 2021, 11:24 a.m. UTC | #1
Hi Jeff,

I know you're on vacation, therefore I would like to apologize for adding
to your post-vacation notification overload, but...

On Sat, 22 May 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
> new file mode 100644
> index 000000000000..e62901a85b5d
> --- /dev/null
> +++ b/fsmonitor-ipc.c
> @@ -0,0 +1,179 @@
> [...]
> +
> +int fsmonitor_ipc__send_query(const char *since_token,
> +			      struct strbuf *answer)
> +{
> +	int ret = -1;
> +	int tried_to_spawn = 0;
> +	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
> +	struct ipc_client_connection *connection = NULL;
> +	struct ipc_client_connect_options options
> +		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
> +
> +	options.wait_if_busy = 1;
> +	options.wait_if_not_found = 0;
> +
> +	trace2_region_enter("fsm_client", "query", NULL);
> +
> +	trace2_data_string("fsm_client", NULL, "query/command",
> +			   since_token);
> +
> +try_again:
> +	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
> +				       &connection);
> +
> +	switch (state) {
> +	case IPC_STATE__LISTENING:
> +		ret = ipc_client_send_command_to_connection(
> +			connection, since_token, strlen(since_token), answer);

Here, `since_token` can be `NULL` (and hence the `strlen(since_token)` can
lead to a segmentation fault). I ran into this situation while `git rebase
-i --autostash` wanted to apply the stashed changes.

Since I picked up your v2 and included it in Git for Windows v2.32.0-rc2,
I needed this hotfix: https://github.com/git-for-windows/git/pull/3241

Thanks,
Dscho
Johannes Schindelin June 14, 2021, 9:23 p.m. UTC | #2
Hi Jeff,

On Wed, 2 Jun 2021, Johannes Schindelin wrote:

> I know you're on vacation, therefore I would like to apologize for adding
> to your post-vacation notification overload, but...

Now that you're back from vacation...

> On Sat, 22 May 2021, Jeff Hostetler via GitGitGadget wrote:
>
> > From: Jeff Hostetler <jeffhost@microsoft.com>
> >
> > diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
> > new file mode 100644
> > index 000000000000..e62901a85b5d
> > --- /dev/null
> > +++ b/fsmonitor-ipc.c
> > @@ -0,0 +1,179 @@
> > [...]
> > +
> > +int fsmonitor_ipc__send_query(const char *since_token,
> > +			      struct strbuf *answer)
> > +{
> > +	int ret = -1;
> > +	int tried_to_spawn = 0;
> > +	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
> > +	struct ipc_client_connection *connection = NULL;
> > +	struct ipc_client_connect_options options
> > +		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
> > +
> > +	options.wait_if_busy = 1;
> > +	options.wait_if_not_found = 0;
> > +
> > +	trace2_region_enter("fsm_client", "query", NULL);
> > +
> > +	trace2_data_string("fsm_client", NULL, "query/command",
> > +			   since_token);
> > +
> > +try_again:
> > +	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
> > +				       &connection);
> > +
> > +	switch (state) {
> > +	case IPC_STATE__LISTENING:
> > +		ret = ipc_client_send_command_to_connection(
> > +			connection, since_token, strlen(since_token), answer);
>
> Here, `since_token` can be `NULL` (and hence the `strlen(since_token)` can
> lead to a segmentation fault). I ran into this situation while `git rebase
> -i --autostash` wanted to apply the stashed changes.
>
> Since I picked up your v2 and included it in Git for Windows v2.32.0-rc2,
> I needed this hotfix: https://github.com/git-for-windows/git/pull/3241

I actually noticed another similar issue and fixed it in time for Git for
Windows v2.32.0, but eventually figured out the actual culprit, with a
much better fix:

-- snip --
commit bc40a560d3c95040b55fd7be6fe5b7012d267f8f
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Wed Jun 9 09:49:50 2021 +0200

    fixup! fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

    In FSMonitor v1, we made sure to only use a valid `since_token` when
    querying the FSMonitor. This condition was accidentally lost in v2, and
    caused segmentation faults uncovered by Scalar's Functional Tests.

    I had tried to fix this in https://github.com/git-for-windows/pull/3241,
    but the fix was incomplete, and I had to follow up with
    https://github.com/git-for-windows/pull/3258. However, it turns out that
    both of them were actually only work-arounds; I should have dug deeper
    to figure out _why_ the `since_token` was no longer guaranteed not to be
    `NULL`, and I finally did.

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/fsmonitor.c b/fsmonitor.c
index 22623fd228f..0b40643442e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -290,8 +290,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");

 	if (r->settings.use_builtin_fsmonitor > 0) {
-		query_success = !fsmonitor_ipc__send_query(
-			istate->fsmonitor_last_update, &query_result);
+		query_success = istate->fsmonitor_last_update &&
+			!fsmonitor_ipc__send_query(istate->fsmonitor_last_update,
+						   &query_result);
 		if (query_success) {
 			/*
 			 * The response contains a series of nul terminated

-- snap --

Would you mind squashing this in when you re-roll?

Ciao,
Dscho
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 21c0bf16672b..23f3b9890acd 100644
--- a/Makefile
+++ b/Makefile
@@ -892,6 +892,7 @@  LIB_OBJS += fetch-pack.o
 LIB_OBJS += fmt-merge-msg.o
 LIB_OBJS += fsck.o
 LIB_OBJS += fsmonitor.o
+LIB_OBJS += fsmonitor-ipc.o
 LIB_OBJS += gettext.o
 LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
new file mode 100644
index 000000000000..e62901a85b5d
--- /dev/null
+++ b/fsmonitor-ipc.c
@@ -0,0 +1,179 @@ 
+#include "cache.h"
+#include "fsmonitor.h"
+#include "simple-ipc.h"
+#include "fsmonitor-ipc.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "trace2.h"
+
+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+
+int fsmonitor_ipc__is_supported(void)
+{
+	return 1;
+}
+
+GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor--daemon.ipc")
+
+enum ipc_active_state fsmonitor_ipc__get_state(void)
+{
+	return ipc_get_active_state(fsmonitor_ipc__get_path());
+}
+
+static int spawn_daemon(void)
+{
+	const char *args[] = { "fsmonitor--daemon", "start", NULL };
+
+	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
+				    "fsmonitor");
+}
+
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer)
+{
+	int ret = -1;
+	int tried_to_spawn = 0;
+	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	trace2_region_enter("fsm_client", "query", NULL);
+
+	trace2_data_string("fsm_client", NULL, "query/command",
+			   since_token);
+
+try_again:
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+
+	switch (state) {
+	case IPC_STATE__LISTENING:
+		ret = ipc_client_send_command_to_connection(
+			connection, since_token, strlen(since_token), answer);
+		ipc_client_close_connection(connection);
+
+		trace2_data_intmax("fsm_client", NULL,
+				   "query/response-length", answer->len);
+
+		if (fsmonitor_is_trivial_response(answer))
+			trace2_data_intmax("fsm_client", NULL,
+					   "query/trivial-response", 1);
+
+		goto done;
+
+	case IPC_STATE__NOT_LISTENING:
+		ret = error(_("fsmonitor_ipc__send_query: daemon not available"));
+		goto done;
+
+	case IPC_STATE__PATH_NOT_FOUND:
+		if (tried_to_spawn)
+			goto done;
+
+		tried_to_spawn++;
+		if (spawn_daemon())
+			goto done;
+
+		/*
+		 * Try again, but this time give the daemon a chance to
+		 * actually create the pipe/socket.
+		 *
+		 * Granted, the daemon just started so it can't possibly have
+		 * any FS cached yet, so we'll always get a trivial answer.
+		 * BUT the answer should include a new token that can serve
+		 * as the basis for subsequent requests.
+		 */
+		options.wait_if_not_found = 1;
+		goto try_again;
+
+	case IPC_STATE__INVALID_PATH:
+		ret = error(_("fsmonitor_ipc__send_query: invalid path '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+
+	case IPC_STATE__OTHER_ERROR:
+	default:
+		ret = error(_("fsmonitor_ipc__send_query: unspecified error on '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+	}
+
+done:
+	trace2_region_leave("fsm_client", "query", NULL);
+
+	return ret;
+}
+
+int fsmonitor_ipc__send_command(const char *command,
+				struct strbuf *answer)
+{
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+	int ret;
+	enum ipc_active_state state;
+
+	strbuf_reset(answer);
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+	if (state != IPC_STATE__LISTENING) {
+		die("fsmonitor--daemon is not running");
+		return -1;
+	}
+
+	ret = ipc_client_send_command_to_connection(connection,
+						    command, strlen(command),
+						    answer);
+	ipc_client_close_connection(connection);
+
+	if (ret == -1) {
+		die("could not send '%s' command to fsmonitor--daemon",
+		    command);
+		return -1;
+	}
+
+	return 0;
+}
+
+#else
+
+/*
+ * A trivial implementation of the fsmonitor_ipc__ API for unsupported
+ * platforms.
+ */
+
+int fsmonitor_ipc__is_supported(void)
+{
+	return 0;
+}
+
+const char *fsmonitor_ipc__get_path(void)
+{
+	return NULL;
+}
+
+enum ipc_active_state fsmonitor_ipc__get_state(void)
+{
+	return IPC_STATE__OTHER_ERROR;
+}
+
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer)
+{
+	return -1;
+}
+
+int fsmonitor_ipc__send_command(const char *command,
+				struct strbuf *answer)
+{
+	return -1;
+}
+
+#endif
diff --git a/fsmonitor-ipc.h b/fsmonitor-ipc.h
new file mode 100644
index 000000000000..837c5e5b64ad
--- /dev/null
+++ b/fsmonitor-ipc.h
@@ -0,0 +1,48 @@ 
+#ifndef FSMONITOR_IPC_H
+#define FSMONITOR_IPC_H
+
+/*
+ * Returns true if built-in file system monitor daemon is defined
+ * for this platform.
+ */
+int fsmonitor_ipc__is_supported(void);
+
+/*
+ * Returns the pathname to the IPC named pipe or Unix domain socket
+ * where a `git-fsmonitor--daemon` process will listen.  This is a
+ * per-worktree value.
+ *
+ * Returns NULL if the daemon is not supported on this platform.
+ */
+const char *fsmonitor_ipc__get_path(void);
+
+/*
+ * Try to determine whether there is a `git-fsmonitor--daemon` process
+ * listening on the IPC pipe/socket.
+ */
+enum ipc_active_state fsmonitor_ipc__get_state(void);
+
+/*
+ * Connect to a `git-fsmonitor--daemon` process via simple-ipc
+ * and ask for the set of changed files since the given token.
+ *
+ * This DOES NOT use the hook interface.
+ *
+ * Spawn a daemon process in the background if necessary.
+ *
+ * Returns -1 on error; 0 on success.
+ */
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer);
+
+/*
+ * Connect to a `git-fsmonitor--daemon` process via simple-ipc and
+ * send a command verb.  If no daemon is available, we DO NOT try to
+ * start one.
+ *
+ * Returns -1 on error; 0 on success.
+ */
+int fsmonitor_ipc__send_command(const char *command,
+				struct strbuf *answer);
+
+#endif /* FSMONITOR_IPC_H */