Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-23 15:12:28
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
+ 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");There was a discussion on v1 about the "default" being redundant here and hiding future compiler checks, this is another "not sure what you thought of that" case (per [1]). Interestingly in this case if I drop the "default" my local gcc uncharacteristically complains about a missing "return" in this function, but clang doesn't. I needed to add a BUG() to shut up the former. Maybe I'm wrong, but perhaps it's a sign of some deeper trouble. This is with gcc/clang 10.2.1-6/11.0.1-2. 1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/ (local) I played with the diff below on top of this, I can't remember if it was noted already, but the way you declare function ptrs and use them isn't the usual style: -- >8 --
diff --git a/run-command.c b/run-command.c
index 76bbef9d96d..5c831545201 100644
--- a/run-command.c
+++ b/run-command.c@@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir) } enum start_bg_result start_bg_command(struct child_process *cmd, - start_bg_wait_cb *wait_cb, + start_bg_wait_cb wait_cb, void *cb_data, unsigned int timeout_sec) {
diff --git a/run-command.h b/run-command.h
index 17b1b80c3d7..e8a637d1967 100644
--- a/run-command.h
+++ b/run-command.h@@ -527,7 +527,7 @@ enum start_bg_result { * 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)(const struct child_process *cmd, void *cb_data); +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
@@ -549,7 +549,7 @@ typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data); * 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, + start_bg_wait_cb wait_cb, void *cb_data, unsigned int timeout_sec);
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 28365ff85b6..82dc2906a2a 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(const struct child_process *cp, void *cb_data) +static int bg_wait_cb(const struct child_process *cp, void *cb_data, int foo) { int s = ipc_get_active_state(cl_args.path);
@@ -319,9 +317,8 @@ static int daemon__start_server(void) switch (sbgr) { case SBGR_READY: return 0; - - default: case SBGR_ERROR: + return 0; case SBGR_CB_ERROR: return error("daemon failed to start");
@@ -331,6 +328,7 @@ static int daemon__start_server(void) case SBGR_DIED: return error("daemon terminated"); } + BUG("unreachable"); } /*