Stefan Beller <sbel...@google.com> writes:
> If the `get_next_task` did not explicitly called child_process_init
> and only filled in some fields, there may have been some stale data
> in the child process. This is hard to debug and also adds a review
> burden for each new user of that API. To improve the situation, we
> pass only cleanly initialized child structs to the get_next_task.
>
> As an invariant you can now assume any child not in use is
> cleaned up and ready for its next reuse.
>
> Signed-off-by: Stefan Beller <sbel...@google.com>
> ---
> run-command.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index b9363da..a5ef874 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)
> argv_array_init(&child->env_array);
> }
>
> +void child_process_deinit(struct child_process *child)
> +{
> + argv_array_clear(&child->args);
> + argv_array_clear(&child->env_array);
> +}
> +
Is this necessary (and is it necessary to make it global)?
I thought that finish_command() already clears them....
... goes and looks ...
Ahh, of course, pp_*() functions do use start_command() but do not
use finish_command(), which sort of breaks symmetry, but that cannot
be helped. Because we want to wait for any of the multiple tasks
running, we cannot call finish_command() that explicitly says "I
want to wait for this one to finish".
And that is why you already have two calls to array-clear inside
collect_finished(), just after calling task_finished().
And of course we already have these array-clear calls in
finish_command().
So I agree that deinit helper should exist, but
* it should be file-scope static;
* it should be called by finish_command(); and
* if you are calling it from collect_finished(), then existing
calls to array-clear should go.
Other than that, this looks good.
Thansk.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
