Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 21, 2015, 20:30 UTC
- Message-ID
- <xmqqh9lkufpt.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqq37x56sz6.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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@google.com> 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@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- 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