| Message ID | 57f29feaadb4a732892cd193b2a1d3c838f09421.1631738177.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Builtin FSMonitor Part 1 | expand |
On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > > 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@microsoft.com> > --- > 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
On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote: [...] > + default: > + case SBGR_ERROR: > + case SBGR_CB_ERROR: > + return error("daemon failed to start"); > > - /* > - * We don't care about the WEXITSTATUS() nor > - * any of the WIF*(status) values because > - * `cmd__simple_ipc()` does the `!!result` > - * trick on all function return values. > - * > - * So it is sufficient to just report the > - * early shutdown as an error. > - */ > - return error(_("daemon failed to start")); > - } > + case SBGR_TIMEOUT: > + return error("daemon not online yet"); > > - else > - return error(_("waitpid is confused")); > + case SBGR_DIED: > + return error("daemon terminated"); > } > } It's not mentioned in the commit message, but the while-we're-at-it dropping of _() makes sense here, it shouldn't have been used in a test helper to begin with. I.e. translators don't need to be translating stuff purely internal to the test suite.
On 9/16/21 1:06 AM, Taylor Blau wrote: > On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote: >> From: Jeff Hostetler <jeffhost@microsoft.com> >> >> 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@microsoft.com> >> --- >> 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(). I use that trick to get the compiler to give me a compiler error at the point of the function declaration. For example, If I add an arg to the function that doesn't match what's in the prototype definition, I get: t/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb' static int bg_wait_cb(const struct child_process *cp, void *cb_data, int foo) ^ t/helper/test-simple-ipc.c:278:25: note: previous declaration is here static start_bg_wait_cb bg_wait_cb; ^ 1 error generated. Yes, we may get an error when the function pointer is referenced in start_bg_command() or if we're using it to initialize a vtable or something, but those errors are further away from the actual error (and sometimes they can be a little cryptic). Also, it helps document that this function's signature is predefined for a reason. It's a quirky trick I know, but it has served me well over the years. > > 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(). Do all compilers now handle switching over an enum and detect unhandled cases? Once upon a time that wasn't the case IIRC. >... > > Ditto. > > Thanks, > Taylor >
On Fri, Sep 17 2021, Jeff Hostetler wrote: > On 9/16/21 1:06 AM, Taylor Blau wrote: >> On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote: >>> From: Jeff Hostetler <jeffhost@microsoft.com> >>> >>> 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@microsoft.com> >>> --- >>> 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(). > > I use that trick to get the compiler to give me a compiler error at the > point of the function declaration. > > For example, If I add an arg to the function that doesn't match what's > in the prototype definition, I get: > > t/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb' > static int bg_wait_cb(const struct child_process *cp, void *cb_data, > int foo) > ^ > t/helper/test-simple-ipc.c:278:25: note: previous declaration is here > static start_bg_wait_cb bg_wait_cb; > ^ > 1 error generated. > > Yes, we may get an error when the function pointer is referenced in > start_bg_command() or if we're using it to initialize a vtable or > something, but those errors are further away from the actual error > (and sometimes they can be a little cryptic). > > Also, it helps document that this function's signature is predefined > for a reason. > > It's a quirky trick I know, but it has served me well over the years. I haven't seen this idiom before. I think it's best to avoid patterns designed to massage messages out of any specific compilers/versions. It seems inevitable that it'll either be counter-productive or redundant. Here with clang v11 doing this makes the warning worse. I.e. without the forward declaration: t/helper/test-simple-ipc.c:315:31: error: incompatible function pointer types passing 'int (void *, const struct child_process *, int)' to parameter of type ' start_bg_wait_cb *' (aka 'int (*)(void *, const struct child_process *)') [-Werror,-Wincompatible-function-pointer-types] sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec); ^~~~~~~~~~ ./run-command.h:564:29: note: passing argument to parameter 'wait_cb' here start_bg_wait_cb *wait_cb, ^ 1 error generated. I.e. we get the specific warning category for this type of error (-Werror,-Wincompatible-function-pointer-types), and we're pointed at the caller in question (which to be fair, it seems you don't prefer), but also a reference to the run-command.h definition. Most importantly, we get quoted what the type is/should be, which is missing with the forward declaration. It's the equivalent of saying "you did bad!" instead of "you did bad X, do Y instead!". >> 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(). > > Do all compilers now handle switching over an enum and detect unhandled > cases? Once upon a time that wasn't the case IIRC. I don't think so, but the ones we widely use do, i.e. clang and gcc at least. For this sort of thing it really doesn't matter if *all* compilers support it, since we'll only need to catch such "missing enum arm" issues with one of them. E.g. in my 338abb0f045 (builtins + test helpers: use return instead of exit() in cmd_*, 2021-06-08) I fixed something that I've only gotten Oracle SunCC to emit (gcc and clang don't detect it), but as long as that one compiler does & someone checks it regularly... By having a "default" case you're hiding that detection from the compilers capable of detecting a logic error in this code, whereas if the compiler can't do that it'll just ignore it.
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; - *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: - return 0; + case IPC_STATE__INVALID_PATH: + case IPC_STATE__OTHER_ERROR: + /* all the time in world won't help */ + return -1; } } -#else -/* - * Conceptually like `daemonize()` but different because Windows does not - * have `fork(2)`. Spawn a normal Windows child process but without the - * limitations of `start_command()` and `finish_command()`. - */ -static int spawn_server(pid_t *pid) -{ - char test_tool_exe[MAX_PATH]; - struct strvec args = STRVEC_INIT; - int in, out; - - GetModuleFileNameA(NULL, test_tool_exe, MAX_PATH); - - in = open("/dev/null", O_RDONLY); - out = open("/dev/null", O_WRONLY); - - strvec_push(&args, test_tool_exe); - strvec_push(&args, "simple-ipc"); - strvec_push(&args, "run-daemon"); - strvec_pushf(&args, "--name=%s", cl_args.path); - strvec_pushf(&args, "--threads=%d", cl_args.nr_threads); - - *pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out); - close(in); - close(out); - - strvec_clear(&args); - if (*pid < 0) - return error(_("could not spawn daemon in the background")); - - return 0; -} -#endif - -/* - * This is adapted from `wait_or_whine()`. Watch the child process and - * let it get started and begin listening for requests on the socket - * before reporting our success. - */ -static int wait_for_server_startup(pid_t pid_child) +static int daemon__start_server(void) { - int status; - pid_t pid_seen; - enum ipc_active_state s; - time_t time_limit, now; + struct child_process cp = CHILD_PROCESS_INIT; + enum start_bg_result sbgr; - time(&time_limit); - time_limit += cl_args.max_wait_sec; + strvec_push(&cp.args, "test-tool"); + strvec_push(&cp.args, "simple-ipc"); + strvec_push(&cp.args, "run-daemon"); + strvec_pushf(&cp.args, "--name=%s", cl_args.path); + strvec_pushf(&cp.args, "--threads=%d", cl_args.nr_threads); - for (;;) { - pid_seen = waitpid(pid_child, &status, WNOHANG); + cp.no_stdin = 1; + cp.no_stdout = 1; + cp.no_stderr = 1; - if (pid_seen == -1) - return error_errno(_("waitpid failed")); + sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec); - else if (pid_seen == 0) { - /* - * The child is still running (this should be - * the normal case). Try to connect to it on - * the socket and see if it is ready for - * business. - * - * If there is another daemon already running, - * our child will fail to start (possibly - * after a timeout on the lock), but we don't - * care (who responds) if the socket is live. - */ - s = ipc_get_active_state(cl_args.path); - if (s == IPC_STATE__LISTENING) - return 0; - - time(&now); - if (now > time_limit) - return error(_("daemon not online yet")); - - continue; - } + switch (sbgr) { + case SBGR_READY: + return 0; - 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: + case SBGR_ERROR: + case SBGR_CB_ERROR: + return error("daemon failed to start"); - /* - * We don't care about the WEXITSTATUS() nor - * any of the WIF*(status) values because - * `cmd__simple_ipc()` does the `!!result` - * trick on all function return values. - * - * So it is sufficient to just report the - * early shutdown as an error. - */ - return error(_("daemon failed to start")); - } + case SBGR_TIMEOUT: + return error("daemon not online yet"); - else - return error(_("waitpid is confused")); + case SBGR_DIED: + return error("daemon terminated"); } } -/* - * This process will start a simple-ipc server in a background process and - * wait for it to become ready. This is like `daemonize()` but gives us - * more control and better error reporting (and makes it easier to write - * unit tests). - */ -static int daemon__start_server(void) -{ - pid_t pid_child; - int ret; - - /* - * Run the actual daemon in a background process. - */ - ret = spawn_server(&pid_child); - if (pid_child <= 0) - return ret; - - /* - * Let the parent wait for the child process to get started - * and begin listening for requests on the socket. - */ - ret = wait_for_server_startup(pid_child); - - return ret; -} - /* * This process will run a quick probe to see if a simple-ipc server * is active on this path.