--- v1
+++ v2
@@ -1,41 +1,16 @@
-In July I sent the V3 version [3] of my Builtin FSMonitor series [1,2,3].
-This has been in "seen" as branch jh/builtin-fsmonitor. At 34 commits, it
-was already a little too big to easily review. With the feed back on [3] and
-from people using the experimental version that we shipped with
-git-for-windows v2.32 and v2.33, I've prepared a new V4 version. This can be
-seen in [4].
-
-However, this new version currently contains 58 commits and is way too big
-to be submitted as is. So I would like to close or discard the original V3
-branch and submit V4 in pieces to make it easier to review.
-
-Here is Part 1 of (what would be V4 of) my Builtin FSMonitor series.
-
-Part 1 contains:
-
- * A fix for a memory leak in the Trace2 code. (This was independently
- reported in last week in "ab/tr2-leaks-and-fixes".)
- * Various cleanups in the simple-ipc layer.
- * A new start_bg_command() function to launch a command into the background
- and wait for it to start. And a refactored consumer of it in
- test-simple-ipc. There was a large discussion on commit 14/34 in V3 [5
- thru 6] about the large ifdef'd blocks of platform specific code to spawn
- background commands, trace2 handling, and duplicated code in
- fsmonitor--daemon.c and in test-simple-ipc. That has all been addressed
- here.
-
-[1]
-https://lore.kernel.org/git/pull.923.git.1617291666.gitgitgadget@gmail.com/
-[2]
-https://lore.kernel.org/git/pull.923.v2.git.1621691828.gitgitgadget@gmail.com/
-[3]
-https://lore.kernel.org/git/pull.923.v3.git.1625150864.gitgitgadget@gmail.com/
-[4] https://github.com/gitgitgadget/git/pull/923 [5]
-https://lore.kernel.org/git/9fe902aad87f1192705fb69ea212a2d066d0286d.1625150864.git.gitgitgadget@gmail.com/
-[6] https://lore.kernel.org/git/87tukovidd.fsf@evledraar.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: fix memory leak of thread name
+ 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
@@ -43,19 +18,312 @@
run-command: create start_bg_command
t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command
- compat/simple-ipc/ipc-unix-socket.c | 14 +-
- compat/simple-ipc/ipc-win32.c | 176 +++++++++++++++++++--
- run-command.c | 123 +++++++++++++++
- run-command.h | 48 ++++++
- simple-ipc.h | 21 +--
- t/helper/test-simple-ipc.c | 227 ++++++++--------------------
- trace2/tr2_tls.c | 1 +
- 7 files changed, 416 insertions(+), 194 deletions(-)
+ 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-v1
-Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v1
+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