From: Taylor Blau <me@ttaylorr.com>
To: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 6/7] run-command: create start_bg_command
Date: Thu, 16 Sep 2021 00:53:07 -0400 [thread overview]
Message-ID: <YULNs166fGOfVUVy@nand.local> (raw)
In-Reply-To: <f97038a563d889d740a7e968fcbdfaadb41e2008.1631738177.git.gitgitgadget@gmail.com>
On Wed, Sep 15, 2021 at 08:36:16PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create a variation of `run_command()` and `start_command()` to launch a command
> into the background and optionally wait for it to become "ready" before returning.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
> run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
> run-command.h | 48 ++++++++++++++++++++
> 2 files changed, 171 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 3e4e082e94d..fe75fd08f74 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
> }
> strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
> }
> +
> +enum start_bg_result start_bg_command(struct child_process *cmd,
> + start_bg_wait_cb *wait_cb,
> + void *cb_data,
> + unsigned int timeout_sec)
> +{
> + enum start_bg_result sbgr = SBGR_ERROR;
> + int ret;
> + int wait_status;
> + pid_t pid_seen;
> + 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.
> + */
> + cmd->clean_on_exit = 0;
> +
> + ret = start_command(cmd);
> + if (ret) {
> + /*
> + * We assume that if `start_command()` fails, we
> + * either get a complete `trace2_child_start() /
> + * trace2_child_exit()` pair or it fails before the
> + * `trace2_child_start()` is emitted, so we do not
> + * need to worry about it here.
> + *
> + * We also assume that `start_command()` does not add
> + * us to the cleanup list. And that it calls
> + * calls `child_process_clear()`.
> + */
> + sbgr = SBGR_ERROR;
> + goto done;
> + }
> +
> + time(&time_limit);
> + time_limit += timeout_sec;
This jumped out to me as unsafe, since POSIX guarantees time_t to be an
integral value holding a number of seconds (so += timeout_sec is safe
there), but it isn't in the C standard.
But we have lots of other examples of adding a number of seconds
directly the value filled in by time(2), so I think this is fine.
> +
> +wait:
> + pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
> +
> + if (pid_seen == 0) {
Small nit, probably better to write this as if (!pid_seen), but not
worth a reroll alone.
> + /*
> + * 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.
> + */
This comment is simple and informative, thank you!
> + ret = (*wait_cb)(cb_data, cmd);
> + 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?
Probably. Maybe trace2_child_orphaned() or trace2_child_background()?
> + */
> + sbgr = SBGR_READY;
> + goto done;
> + } else if (ret > 0) {
> + time_t now;
> +
> + time(&now);
> + if (now < time_limit)
> + goto wait;
> +
> + /*
> + * 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?
> + */
> + 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?
> + */
> + sbgr = SBGR_CB_ERROR;
> + goto done;
> + }
OK, so assuming that the child is running, then we ask wait_cb what to
do. Returning zero from the callback means to background it, a positive
value means to give it more time, and negative means to cause an error.
And those match the documentation below, good.
> + if (pid_seen == cmd->pid) {
This could be an "else if", no?
> + int child_code = -1;
> +
> + /*
> + * The child started, but exited or was terminated
> + * before becoming "ready".
> + *
> + * We try to match the behavior of `wait_or_whine()`
> + * and convert the child's status to a return code for
> + * tracing purposes and emit the `trace2_child_exit()`
> + * event.
> + */
> + if (WIFEXITED(wait_status))
> + child_code = WEXITSTATUS(wait_status);
> + else if (WIFSIGNALED(wait_status))
> + child_code = WTERMSIG(wait_status) + 128;
Do we care about emitting the same error (when it was signaled with
something other than SIGINT/SIGQUIT/SIGPIPE) as is reported by
wait_or_whine()?
If we want that error here, too, we could probably share the same code
here from here and in wait_or_whine(). I would probably write something
like:
static int handle_awaited_status(int status, int *code)
{
if (WIFSIGNALED(status)) {
*code = WTERMSIG(status);
if (*code != SIGINT && *code != SIGQUIT && *code != SIGPIPE)
error("%s died of signal %d", argv0, *code);
/*
* This return value is chosen so that code & 0xff
* mimics the exit code that a POSIX shell would report for
* a program that died from this signal.
*/
*code += 128;
return 1;
} else if (WIFEXITED(status)) {
*code = WEXITSTATUS(status);
return 1;
}
return 0;
}
so that we could call it in wait_or_whine() like:
} else if (!handle_awaited_status(status, &code)) {
error("waitpid is confused (%s)", argv0);
}
and similarly here in this new function. Alternatively, if we don't want
that error, then it may help future readers to add a short comment
explaining why not.
> +/**
> + * Callback used by `start_bg_command()` to ask whether the
> + * child process is ready or needs more time to become ready.
> + *
> + * 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.
> + */
> +typedef int(start_bg_wait_cb)(void *cb_data,
> + const struct child_process *cmd);
Nitpicking, but typically I would assume that the "extra" void pointer
is the last argument in a callback. It definitely does not matter,
though.
Thanks,
Taylor
next prev parent reply other threads:[~2021-09-16 4:53 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 [this message]
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 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
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=YULNs166fGOfVUVy@nand.local \
--to=me@ttaylorr.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jeffhost@microsoft.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).