Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
On Wed, Oct 21, 2015 at 1:30 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> 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. > > I'll queue this instead (the above squashed in and description > corrected). > Thanks, Stefan -- 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
Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
Junio C Hamano writes:
> 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.
I'll queue this instead (the above squashed in and description
corrected).
-- >8 --
From: Stefan Beller
Date: Tue, 20 Oct 2015 15:43:44 -0700
Subject: [PATCH] run-command: clear leftover state from child_process structure
pp_start_one() function finds an unused child_process structure
passes it to start_command(), but the structure may have already
been used earlier. finish_command() has code to clear leftover
states in the structure so that it can be reused, but the parallel
execution machinery does not (and cannot) use it, and instead has
its own pp_collect_finished(). This function, after culling a
process that has just finished, forgot to clear the child_process
structure before marking it ready for reuse.
Introduce child_process_deinit() helper function that clears two
instances of argv_array (one for arg, the other for env) in the
structure, make the existing codepaths that clear them call the
helper instead (which in turn will make sure that we will not leak
resources later even when we add new fields to the structure), and
also add a call to it in pp_collect_finished() before the function
marks the structure read for reuse.
Signed-off-by: Stefan Beller
Signed-off-by: Junio C Hamano
---
run-command.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/run-command.c b/run-command.c
index b9363da..684ccee 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);
}
+static void child_process_deinit(struct child_process *child)
+{
+ argv_array_clear(&child->args);
+ argv_array_clear(&child->env_array);
+}
+
struct child_to_clean {
pid_t pid;
struct child_to_clean *next;
@@ -338,8 +344,7 @@ int start_command(struct child_process *cmd)
fail_pipe:
error("cannot create %s pipe for %s: %s",
str, cmd->argv[0], strerror(failed_errno));
- argv_array_clear(&cmd->args);
- argv_array_clear(&cmd->env_array);
+ child_process_deinit(cmd);
errno = failed_errno;
return -1;
}
@@ -525,8 +530,7 @@ fail_pipe:
close_pair(fderr);
else if (cmd->err)
close(cmd->err);
- argv_array_clear(&cmd->args);
- argv_array_clear(&cmd->env_array);
+ child_process_deinit(cmd);
errno = failed_errno;
return -1;
}
@@ -552,8 +556,7 @@ fail_pipe:
int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
- argv_array_clear(&cmd->args);
- argv_array_clear(&cmd->env_array);
+ child_process_deinit(cmd);
return ret;
}
@@ -962,6 +965,7 @@ static struct parallel_processes *pp_init(int n,
for (i = 0; i < n; i++) {
strbuf_init(&pp->children[i].err, 0);
+ child_process_init(&pp->children[i].process);
pp->pfd[i].events = POLLIN;
pp->pfd[i].fd = -1;
}
@@ -973,8 +977,10 @@ static void pp_cleanup(struct parallel_processes *pp)
{
int i;
- for (i = 0; i < pp->max_processes; i++)
+ for (i = 0; i < pp->max_processes; i++) {
strbuf_release(&pp->children[i].err);
+ child_process_deinit(&pp->children[i].process);
+ }
free(pp->children);
free(pp->pfd);
@@ -1128,12 +1134,11 @@ static int pp_collect_finished(struct
parallel_processes *pp)
&pp->children[i].data))
result = 1;
- argv_array_clear(&pp->children[i].process.args);
- argv_array_clear(&pp->children[i].process.env_array);
-
pp->nr_processes--;
pp->children[i].in_use = 0;
pp->pfd[i].fd = -1;
+ child_process_deinit(&pp->children[i].process);
+ child_process_init(&pp->children[i].process);
if (i != pp->output_owner) {
strbuf_addbuf(&pp->buffered_output,
&pp->children[i].err);
--
2.6.2-394-gc0a4d5b
--
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
Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
Stefan Beller 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
> ---
> 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
Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
Stefan Beller 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
> ---
> 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.
Thanks.
--
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
