Junio C Hamano <gitster@xxxxxxxxx> 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 <sbeller@xxxxxxxxxx>
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 <sbeller@xxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html