| Message ID | patch-v3-15.15-39a20be0cbb-20221012T205712Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0b0ab95f17d5eb71d2552e2c3b0806e2ed87beca |
| Headers | show |
| Series | run-command API: pass functions & opts via struct | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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
diff --git a/run-command.c b/run-command.c index ce7966394d4..c772acd7431 100644 --- a/run-command.c +++ b/run-command.c @@ -1497,7 +1497,6 @@ enum child_state { }; struct parallel_processes { - const size_t max_processes; size_t nr_processes; struct { @@ -1518,24 +1517,38 @@ struct parallel_processes { struct strbuf buffered_output; /* of finished children */ }; -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; +}; + +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); } }
As with the *_fn members removed in a preceding commit, let's not copy the "processes" member of the "struct run_process_parallel_opts" over to the "struct parallel_processes". 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- run-command.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)