[PATCH v2 0/7] Builtin FSMonitor Part 1

["Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>]

Here is V2 of Part 1 of my Builtin FSMonitor series.

Changes since V1 include:

 * Drop the Trace2 memory leak.
 * Added a new "child_ready" event to Trace2 as an alternative to the
   "child_exit" event for background processes.
 * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use
   the new "child_ready" event.
 * Various minor code and documentation cleanups.

Jeff Hostetler (7):
  trace2: add trace2_child_ready() to report on background children
  simple-ipc: preparations for supporting binary messages.
  simple-ipc: move definition of ipc_active_state outside of ifdef
  simple-ipc/ipc-win32: add trace2 debugging
  simple-ipc/ipc-win32: add Windows ACL to named pipe
  run-command: create start_bg_command
  t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

 Documentation/technical/api-trace2.txt |  40 +++++
 compat/simple-ipc/ipc-unix-socket.c    |  14 +-
 compat/simple-ipc/ipc-win32.c          | 179 +++++++++++++++++--
 run-command.c                          | 129 ++++++++++++++
 run-command.h                          |  57 ++++++
 simple-ipc.h                           |  21 ++-
 t/helper/test-simple-ipc.c             | 233 +++++++------------------
 trace2.c                               |  31 ++++
 trace2.h                               |  25 +++
 trace2/tr2_tgt.h                       |   5 +
 trace2/tr2_tgt_event.c                 |  22 +++
 trace2/tr2_tgt_normal.c                |  14 ++
 trace2/tr2_tgt_perf.c                  |  15 ++
 13 files changed, 587 insertions(+), 198 deletions(-)


base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1040%2Fjeffhostetler%2Fbuiltin-fsmonitor-part1-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1040

Range-diff vs v1:

 1:  5f557caee00 < -:  ----------- trace2: fix memory leak of thread name
 -:  ----------- > 1:  f88e9feff26 trace2: add trace2_child_ready() to report on background children
 2:  7182419f6df ! 2:  258baa0df8c simple-ipc: preparations for supporting binary messages.
     @@ Commit message
      
          Add `command_len` argument to the Simple IPC API.
      
     -    In my original Simple IPC API, I assumed that the request
     -    would always be a null-terminated string of text characters.
     -    The command arg was just a `const char *`.
     +    In my original Simple IPC API, I assumed that the request would always
     +    be a null-terminated string of text characters.  The `command`
     +    argument was just a `const char *`.
      
     -    I found a caller that would like to pass a binary command
     -    to the daemon, so I want to ammend the Simple IPC API to
     -    take `const char *command, size_t command_len` and pass
     -    that to the daemon.  (Really, the first arg should just be
     -    a `void *` or `const unsigned byte *` to make that clearer.)
     +    I found a caller that would like to pass a binary command to the
     +    daemon, so I am amending the Simple IPC API to receive `const char
     +    *command, size_t command_len` arguments.
      
     -    Note, the response side has always been a `struct strbuf`
     -    which includes the buffer and length, so we already support
     -    returning a binary answer.  (Yes, it feels a little weird
     -    returning a binary buffer in a `strbuf`, but it works.)
     +    I considered changing the `command` argument to be a `void *`, but the
     +    IPC layer simply passes it to the pkt-line layer which takes a `const
     +    char *`, so to avoid confusion I left it as is.
     +
     +    Note, the response side has always been a `struct strbuf` which
     +    includes the buffer and length, so we already support returning a
     +    binary answer.  (Yes, it feels a little weird returning a binary
     +    buffer in a `strbuf`, but it works.)
      
          Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
      
 3:  7de207828ca = 3:  c94b4cbcbf2 simple-ipc: move definition of ipc_active_state outside of ifdef
 4:  30b7bb247c3 ! 4:  82b6ce0dd6a simple-ipc/ipc-win32: add trace2 debugging
     @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state get_active_state(wch
       }
       
      @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state connect_to_server(
     - 				if (GetLastError() == ERROR_SEM_TIMEOUT)
     + 			t_start_ms = (DWORD)(getnanotime() / 1000000);
     + 
     + 			if (!WaitNamedPipeW(wpath, timeout_ms)) {
     +-				if (GetLastError() == ERROR_SEM_TIMEOUT)
     ++				DWORD gleWait = GetLastError();
     ++
     ++				if (gleWait == ERROR_SEM_TIMEOUT)
       					return IPC_STATE__NOT_LISTENING;
       
     -+				gle = GetLastError();
      +				trace2_data_intmax("ipc-debug", NULL,
      +						   "connect/waitpipe/gle",
     -+						   (intmax_t)gle);
     ++						   (intmax_t)gleWait);
      +
       				return IPC_STATE__OTHER_ERROR;
       			}
 5:  5eadf719295 = 5:  faf6034848e simple-ipc/ipc-win32: add Windows ACL to named pipe
 6:  f97038a563d ! 6:  0822118c4b5 run-command: create start_bg_command
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char
      +	time_t time_limit;
      +
      +	/*
     -+	 * Silently disallow child cleanup -- even if requested.
     -+	 * The child process should persist in the background
     -+	 * and possibly/probably after this process exits.  That
     -+	 * is, don't kill the child during our atexit routine.
     ++	 * We do not allow clean-on-exit because the child process
     ++	 * should persist in the background and possibly/probably
     ++	 * after this process exits.  So we don't want to kill the
     ++	 * child during our atexit routine.
      +	 */
     -+	cmd->clean_on_exit = 0;
     ++	if (cmd->clean_on_exit)
     ++		BUG("start_bg_command() does not allow non-zero clean_on_exit");
     ++
     ++	if (!cmd->trace2_child_class)
     ++		cmd->trace2_child_class = "background";
      +
      +	ret = start_command(cmd);
      +	if (ret) {
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char
      +wait:
      +	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
      +
     -+	if (pid_seen == 0) {
     ++	if (!pid_seen) {
      +		/*
      +		 * The child is currently running.  Ask the callback
      +		 * if the child is ready to do work or whether we
      +		 * should keep waiting for it to boot up.
      +		 */
     -+		ret = (*wait_cb)(cb_data, cmd);
     ++		ret = (*wait_cb)(cmd, cb_data);
      +		if (!ret) {
      +			/*
      +			 * The child is running and "ready".
     -+			 *
     -+			 * NEEDSWORK: As we prepare to orphan (release to
     -+			 * the background) this child, it is not appropriate
     -+			 * to emit a `trace2_child_exit()` event.  Should we
     -+			 * create a new event for this case?
      +			 */
     ++			trace2_child_ready(cmd, "ready");
      +			sbgr = SBGR_READY;
      +			goto done;
      +		} else if (ret > 0) {
     ++			/*
     ++			 * The callback said to give it more time to boot up
     ++			 * (subject to our timeout limit).
     ++			 */
      +			time_t now;
      +
      +			time(&now);
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char
      +			 * Our timeout has expired.  We don't try to
      +			 * kill the child, but rather let it continue
      +			 * (hopefully) trying to startup.
     -+			 *
     -+			 * NEEDSWORK: Like the "ready" case, should we
     -+			 * log a custom child-something Trace2 event here?
      +			 */
     ++			trace2_child_ready(cmd, "timeout");
      +			sbgr = SBGR_TIMEOUT;
      +			goto done;
      +		} else {
      +			/*
     -+			 * The cb gave up on this child.
     -+			 *
     -+			 * NEEDSWORK: Like above, should we log a custom
     -+			 * Trace2 child-something event here?
     ++			 * The cb gave up on this child.  It is still running,
     ++			 * but our cb got an error trying to probe it.
      +			 */
     ++			trace2_child_ready(cmd, "error");
      +			sbgr = SBGR_CB_ERROR;
      +			goto done;
      +		}
      +	}
      +
     -+	if (pid_seen == cmd->pid) {
     ++	else if (pid_seen == cmd->pid) {
      +		int child_code = -1;
      +
      +		/*
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char
      +		 * before becoming "ready".
      +		 *
      +		 * We try to match the behavior of `wait_or_whine()`
     ++		 * WRT the handling of WIFSIGNALED() and WIFEXITED()
      +		 * and convert the child's status to a return code for
      +		 * tracing purposes and emit the `trace2_child_exit()`
      +		 * event.
     ++		 *
     ++		 * We do not want the wait_or_whine() error message
     ++		 * because we will be called by client-side library
     ++		 * routines.
      +		 */
      +		if (WIFEXITED(wait_status))
      +			child_code = WEXITSTATUS(wait_status);
     @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char
      +		goto done;
      +	}
      +
     -+	if (pid_seen < 0 && errno == EINTR)
     ++	else if (pid_seen < 0 && errno == EINTR)
      +		goto wait;
      +
      +	trace2_child_exit(cmd, -1);
     @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai
       void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
       
      +/**
     -+ * Possible return values for `start_bg_command()`.
     ++ * Possible return values for start_bg_command().
      + */
      +enum start_bg_result {
      +	/* child process is "ready" */
     @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai
      +};
      +
      +/**
     -+ * Callback used by `start_bg_command()` to ask whether the
     -+ * child process is ready or needs more time to become ready.
     ++ * Callback used by start_bg_command() to ask whether the
     ++ * child process is ready or needs more time to become "ready".
     ++ *
     ++ * The callback will receive the cmd and cb_data arguments given to
     ++ * start_bg_command().
      + *
      + * Returns 1 is child needs more time (subject to the requested timeout).
     -+ * Returns 0 if child is ready.
     -+ * Returns -1 on any error and cause `start_bg_command()` to also error out.
     ++ * Returns 0 if child is "ready".
     ++ * Returns -1 on any error and cause start_bg_command() to also error out.
      + */
     -+typedef int(start_bg_wait_cb)(void *cb_data,
     -+			      const struct child_process *cmd);
     ++typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
      +
      +/**
     -+ * Start a command in the background.  Wait long enough for the child to
     -+ * become "ready".  Capture immediate errors (like failure to start) and
     -+ * any immediate exit status (such as a shutdown/signal before the child
     -+ * became "ready").
     ++ * Start a command in the background.  Wait long enough for the child
     ++ * to become "ready" (as defined by the provided callback).  Capture
     ++ * immediate errors (like failure to start) and any immediate exit
     ++ * status (such as a shutdown/signal before the child became "ready")
     ++ * and return this like start_command().
     ++ *
     ++ * We run a custom wait loop using the provided callback to wait for
     ++ * the child to start and become "ready".  This is limited by the given
     ++ * timeout value.
     ++ *
     ++ * If the child does successfully start and become "ready", we orphan
     ++ * it into the background.
      + *
     -+ * This is a combination of `start_command()` and `finish_command()`, but
     -+ * with a custom `wait_or_whine()` that allows the caller to define when
     -+ * the child is "ready".
     ++ * The caller must not call finish_command().
      + *
     -+ * The caller does not need to call `finish_command()`.
     ++ * The opaque cb_data argument will be forwarded to the callback for
     ++ * any instance data that it might require.  This may be NULL.
      + */
      +enum start_bg_result start_bg_command(struct child_process *cmd,
      +				      start_bg_wait_cb *wait_cb,
 7:  57f29feaadb ! 7:  6b7a058284b t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command
     @@ Commit message
          Convert test helper to use `start_bg_command()` when spawning a server
          daemon in the background rather than blocks of platform-specific code.
      
     +    Also, while here, remove _() translation around error messages since
     +    this is a test helper and not Git code.
     +
          Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
      
       ## t/helper/test-simple-ipc.c ##
     @@ t/helper/test-simple-ipc.c
       #ifndef SUPPORTS_SIMPLE_IPC
       int cmd__simple_ipc(int argc, const char **argv)
      @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)
     + 	 */
     + 	ret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data);
     + 	if (ret == -2)
     +-		error(_("socket/pipe already in use: '%s'"), cl_args.path);
     ++		error("socket/pipe already in use: '%s'", cl_args.path);
     + 	else if (ret == -1)
     +-		error_errno(_("could not start server on: '%s'"), cl_args.path);
     ++		error_errno("could not start server on: '%s'", cl_args.path);
     + 
       	return ret;
       }
       
     @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)
      -		close(1);
      -		close(2);
      -		sanitize_stdfds();
     -+static int bg_wait_cb(void *cb_data, const struct child_process *cp)
     ++static int bg_wait_cb(const struct child_process *cp, void *cb_data)
      +{
      +	int s = ipc_get_active_state(cl_args.path);
       
     @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)
       /*
        * This process will run a quick probe to see if a simple-ipc server
        * is active on this path.
     +@@ t/helper/test-simple-ipc.c: static int client__stop_server(void)
     + 
     + 		time(&now);
     + 		if (now > time_limit)
     +-			return error(_("daemon has not shutdown yet"));
     ++			return error("daemon has not shutdown yet");
     + 	}
     + }
     + 

-- 
gitgitgadget