From: "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Eric Sunshine" <sunshine@sunshineco.com>,
"Taylor Blau" <me@ttaylorr.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Jeff Hostetler" <git@jeffhostetler.com>,
"Carlo Arenas" <carenas@gmail.com>,
"Jeff Hostetler" <jeffhost@microsoft.com>
Subject: [PATCH v2 0/7] Builtin FSMonitor Part 1
Date: Mon, 20 Sep 2021 15:36:11 +0000 [thread overview]
Message-ID: <pull.1040.v2.git.1632152178.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1040.git.1631738177.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
next prev parent reply other threads:[~2021-09-20 15:36 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-15 20:36 [PATCH 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-15 20:36 ` [PATCH 1/7] trace2: fix memory leak of thread name Jeff Hostetler via GitGitGadget
2021-09-15 21:01 ` Junio C Hamano
2021-09-16 4:19 ` Taylor Blau
2021-09-16 5:35 ` Ævar Arnfjörð Bjarmason
2021-09-16 5:43 ` Taylor Blau
2021-09-16 8:01 ` Ævar Arnfjörð Bjarmason
2021-09-16 15:35 ` Jeff Hostetler
2021-09-16 15:47 ` Ævar Arnfjörð Bjarmason
2021-09-16 19:13 ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-15 20:43 ` Eric Sunshine
2021-09-17 16:52 ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-15 21:06 ` Junio C Hamano
2021-09-17 16:58 ` Jeff Hostetler
2021-09-18 7:03 ` Carlo Arenas
2021-09-20 15:51 ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-16 5:40 ` Ævar Arnfjörð Bjarmason
2021-09-17 17:27 ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-16 5:47 ` Ævar Arnfjörð Bjarmason
2021-09-17 18:10 ` Jeff Hostetler
2021-09-17 19:14 ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16 4:53 ` Taylor Blau
2021-09-16 4:58 ` Taylor Blau
2021-09-16 5:56 ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16 5:06 ` Taylor Blau
2021-09-17 19:41 ` Jeff Hostetler
2021-09-18 8:59 ` Ævar Arnfjörð Bjarmason
2021-09-16 5:55 ` Ævar Arnfjörð Bjarmason
2021-09-20 15:36 ` Jeff Hostetler via GitGitGadget [this message]
2021-09-20 15:36 ` [PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-20 15:36 ` [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-23 15:03 ` Ævar Arnfjörð Bjarmason
2021-09-23 17:58 ` Jeff Hostetler
2021-09-23 18:37 ` Junio C Hamano
2021-11-04 19:46 ` Adam Dinwoodie
2021-11-04 20:14 ` Ramsay Jones
2021-11-08 14:58 ` Jeff Hostetler
2021-11-08 23:59 ` Johannes Schindelin
2021-11-09 18:53 ` Ramsay Jones
2021-11-09 23:01 ` Johannes Schindelin
2021-11-09 23:34 ` Junio C Hamano
2021-11-10 12:27 ` Johannes Schindelin
2021-11-12 8:56 ` Adam Dinwoodie
2021-11-12 16:01 ` Junio C Hamano
2021-11-12 21:33 ` Adam Dinwoodie
2021-11-16 10:56 ` Johannes Schindelin
2021-11-16 11:02 ` Johannes Schindelin
2021-11-13 19:11 ` Ramsay Jones
2021-11-14 19:34 ` Ramsay Jones
2021-11-14 20:10 ` Adam Dinwoodie
2021-09-23 14:33 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Ævar Arnfjörð Bjarmason
2021-09-23 17:12 ` Jeff Hostetler
2021-09-23 20:47 ` Ævar Arnfjörð Bjarmason
2021-09-27 13:37 ` Jeff Hostetler
2021-09-23 17:51 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1040.v2.git.1632152178.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=avarab@gmail.com \
--cc=carenas@gmail.com \
--cc=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=jeffhost@microsoft.com \
--cc=me@ttaylorr.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).