On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
>
> Convert test helper to use `start_bg_command()` when spawning a server
> daemon in the background rather than blocks of platform-specific code.
>
> Signed-off-by: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
> ---
> t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------
> 1 file changed, 40 insertions(+), 153 deletions(-)
>
> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
> index 91345180750..59a950f3b00 100644
> --- a/t/helper/test-simple-ipc.c
> +++ b/t/helper/test-simple-ipc.c
> @@ -9,6 +9,7 @@
> #include "parse-options.h"
> #include "thread-utils.h"
> #include "strvec.h"
> +#include "run-command.h"
>
> #ifndef SUPPORTS_SIMPLE_IPC
> int cmd__simple_ipc(int argc, const char **argv)
> @@ -274,178 +275,64 @@ static int daemon__run_server(void)
> return ret;
> }
>
> -#ifndef GIT_WINDOWS_NATIVE
> -/*
> - * This is adapted from `daemonize()`. Use `fork()` to directly create and
> - * run the daemon in a child process.
> - */
> -static int spawn_server(pid_t *pid)
> -{
> - struct ipc_server_opts opts = {
> - .nr_threads = cl_args.nr_threads,
> - };
> +static start_bg_wait_cb bg_wait_cb;
This whole patch is delightful to read, as the new implementation is so
much cleaner as a result of the earlier work in this series.
Am I correct in assuming that this is to encourage a compiler error if
bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I
think we are already getting that by trying to pass bg_wait_cb to
start_bg_command().
E.g., applying this (intentionally broken) diff on top:
--- 8< ---
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 59a950f3b0..3aed787206 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -275,9 +275,7 @@ static int daemon__run_server(void)
return ret;
}
-static start_bg_wait_cb bg_wait_cb;
-
-static int bg_wait_cb(void *cb_data, const struct child_process *cp)
+static int bg_wait_cb(const void *cb_data, const struct child_process *cp)
{
int s = ipc_get_active_state(cl_args.path);
--- >8 ---
and then compiling still warns of a mismatched type when calling
start_bg_command().
> - *pid = fork();
> -
> - switch (*pid) {
> - case 0:
> - if (setsid() == -1)
> - error_errno(_("setsid failed"));
> - close(0);
> - close(1);
> - close(2);
> - sanitize_stdfds();
> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)
> +{
> + int s = ipc_get_active_state(cl_args.path);
>
> - return ipc_server_run(cl_args.path, &opts, test_app_cb,
> - (void*)&my_app_data);
> + switch (s) {
> + case IPC_STATE__LISTENING:
> + /* child is "ready" */
> + return 0;
>
> - case -1:
> - return error_errno(_("could not spawn daemon in the background"));
> + case IPC_STATE__NOT_LISTENING:
> + case IPC_STATE__PATH_NOT_FOUND:
> + /* give child more time */
> + return 1;
>
> default:
I'm always a little hesitant to have default cases when switch over enum
types, since it suppresses the warning when there's a new value of that
type. But we already have a similar default in client__probe_server().
> - else if (pid_seen == pid_child) {
> - /*
> - * The new child daemon process shutdown while
> - * it was starting up, so it is not listening
> - * on the socket.
> - *
> - * Try to ping the socket in the odd chance
> - * that another daemon started (or was already
> - * running) while our child was starting.
> - *
> - * Again, we don't care who services the socket.
> - */
> - s = ipc_get_active_state(cl_args.path);
> - if (s == IPC_STATE__LISTENING)
> - return 0;
> + default:
Ditto.
Thanks,
Taylor
