Re: [PATCH v3 15/15] run-command.c: remove "max_processes", add "const" to signal() handler
- From
Glen Choo <chooglen@google.com>
- Date
- Oct 13, 2022, 22:02 UTC
- Message-ID
- <kl6lsfjrz8jt.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <patch-v3-15.15-39a20be0cbb-20221012T205712Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 7 quoted lines
> In this case we need the number of processes for the kill_children() > function, which will be called from a signal handler. To do that > adjust this code added in c553c72eed6 (run-command: add an > asynchronous parallel child processor, 2015-12-15) so that we use a > dedicated "struct parallel_processes_for_signal" for passing data to > the signal handler, in addition to the "struct parallel_process" it'll > now have access to our "opts" variable.
[...]
Show 5 quoted lines
> -static void kill_children(const struct parallel_processes *pp, int signo)
> +struct parallel_processes_for_signal {
> + const struct run_process_parallel_opts *opts;
> + const struct parallel_processes *pp;
> +};(Treat this feedback as non-blocking)
I find this struct a little odd because it isn't specific to kill_children_signal(), rather, this seems like we are saying that "run_process_parallel_opts" and "parallel_processes" belong together. This also seems confirmed by the fact that at the end of the series, we pass the opts almost everywhere - only pp_output() takes "parallel_processes" without "run_process_parallel_opts".
I assume you've already considered this, but I wonder if the code is simpler if we drop 10-15/15, i.e. use options just for end users (allowing us to get rid of the *_tr2() variant too, which is great), but only use "parallel_processes" inside of run-command.c. There's also a tiny benefit of passing one fewer pointer per function, with the tiny cost of one-time copying.
Thoughts?
Show 85 quoted lines
> +
> +static void kill_children(const struct parallel_processes *pp,
> + const struct run_process_parallel_opts *opts,
> + int signo)
> {
> - for (size_t i = 0; i < pp->max_processes; i++)
> + for (size_t i = 0; i < opts->processes; i++)
> if (pp->children[i].state == GIT_CP_WORKING)
> kill(pp->children[i].process.pid, signo);
> }
>
> -static struct parallel_processes *pp_for_signal;
> +static void kill_children_signal(const struct parallel_processes_for_signal *pp_sig,
> + int signo)
> +{
> + kill_children(pp_sig->pp, pp_sig->opts, signo);
> +}
> +
> +static struct parallel_processes_for_signal *pp_for_signal;
>
> static void handle_children_on_signal(int signo)
> {
> - kill_children(pp_for_signal, signo);
> + kill_children_signal(pp_for_signal, signo);
> sigchain_pop(signo);
> raise(signo);
> }
>
> static void pp_init(struct parallel_processes *pp,
> - const struct run_process_parallel_opts *opts)
> + const struct run_process_parallel_opts *opts,
> + struct parallel_processes_for_signal *pp_sig)
> {
> const size_t n = opts->processes;
>
> @@ -1561,7 +1574,9 @@ static void pp_init(struct parallel_processes *pp,
> }
> }
>
> - pp_for_signal = pp;
> + pp_sig->pp = pp;
> + pp_sig->opts = opts;
> + pp_for_signal = pp_sig;
> sigchain_push_common(handle_children_on_signal);
> }
>
> @@ -1759,8 +1774,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> int i, code;
> int output_timeout = 100;
> int spawn_cap = 4;
> + struct parallel_processes_for_signal pp_sig;
> struct parallel_processes pp = {
> - .max_processes = opts->processes,
> .buffered_output = STRBUF_INIT,
> };
> /* options */
> @@ -1772,7 +1787,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> trace2_region_enter_printf(tr2_category, tr2_label, NULL,
> "max:%d", opts->processes);
>
> - pp_init(&pp, opts);
> + pp_init(&pp, opts, &pp_sig);
> while (1) {
> for (i = 0;
> i < spawn_cap && !pp.shutdown &&
> @@ -1783,7 +1798,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> continue;
> if (code < 0) {
> pp.shutdown = 1;
> - kill_children(&pp, -code);
> + kill_children(&pp, opts, -code);
> }
> break;
> }
> @@ -1800,7 +1815,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> if (code) {
> pp.shutdown = 1;
> if (code < 0)
> - kill_children(&pp, -code);
> + kill_children(&pp, opts,-code);
> }
> }
>
> --
> 2.38.0.971.ge79ff6d20e7