Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> 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.
[...]
> -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?
> +
> +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
