Re: [PATCH v2 2/8] run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- May 25, 2022, 13:18 UTC
- Message-ID
- <Yo4sriq2YYtIsB0p@google.com>
- In-Reply-To
- <patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com>
On Wed, May 18, 2022 at 10:05:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
> > Add a new "struct run_process_parallel_opts" to replace the growing > run_processes_parallel() and run_processes_parallel_tr2() argument > lists. This refactoring makes it easier to add new options and > parameters easier. > > The *_tr2() variant of the function was added in ee4512ed481 (trace2: > create new combined trace facility, 2019-02-22), and has subsequently > been used by every caller except t/helper/test-run-command.c. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > ---
[...]
Show 13 quoted lines
> diff --git a/run-command.h b/run-command.h
> index 5bd0c933e80..b0268ed3db1 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -458,6 +458,32 @@ typedef int (*task_finished_fn)(int result,
> void *pp_task_cb);
>
> /**
> + * Options to pass to run_processes_parallel(), { 0 }-initialized
> + * means no options. Fields:
> + *
> + * tr2_category & tr2_label: sets the trace2 category and label for
> + * logging. These must either be unset, or both of them must be set.I see this comment...
Show 17 quoted lines
> + *
> + * jobs: see 'n' in run_processes_parallel() below.
> + *
> + * *_fn & data: see run_processes_parallel() below.
> + */
> +struct run_process_parallel_opts
> +{
> + const char *tr2_category;
> + const char *tr2_label;
> +
> + int jobs;
> +
> + get_next_task_fn get_next_task;
> + start_failure_fn start_failure;
> + task_finished_fn task_finished;
> + void *data;
> +};[moved snippet]
Show 14 quoted lines
> -int run_processes_parallel(int n,
> - get_next_task_fn get_next_task,
> - start_failure_fn start_failure,
> - task_finished_fn task_finished,
> - void *pp_cb)
> +int run_processes_parallel(struct run_process_parallel_opts *opts)
> {
> int i, code;
> int output_timeout = 100;
> int spawn_cap = 4;
> struct parallel_processes pp;
> + const char *tr2_category = opts->tr2_category;
> + const char *tr2_label = opts->tr2_label;
> + const int do_trace2 = tr2_category && tr2_label;...but it's not actually very well enforced here. That is, it seems I can set one or the other but not both, and nothing bad will happen, except that I am wasting my time setting one. If you want to enforce them both to be set, then why not use a BUG()? But otherwise the comment could be reworded, I think.
Show 12 quoted lines
> + const int n = opts->jobs;
>
> - pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
> + if (do_trace2)
> + trace2_region_enter_printf(tr2_category, tr2_label, NULL,
> + "max:%d", ((n < 1) ? online_cpus()
> + : n));
> +
> + pp_init(&pp, opts);
> while (1) {
> for (i = 0;
> i < spawn_cap && !pp.shutdown &&[/moved snippet]
Otherwise, although the number of lines of code is often higher, I find the named initializers in the struct much easier to read at the callsites, so I like this change.
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>