# [PATCH v2 0/3] daemon: plug memory leak 6 messages from 2015-10-31 to 2015-10-31. Participants: René Scharfe, Jeff King, Stefan Beller. Thread: https://gitlist.dev/t/40684 ## René Scharfe, 2015-10-31 09:16 Subject: [PATCH v2 0/3] daemon: plug memory leak Message-ID: <56348709.9080207@web.de> URL: https://gitlist.dev/e/56348709.9080207%40web.de ``` Changes since v1: - Rebased on next, which already has a cleanup function. - Added first patch for renaming it. Rene Scharfe (3): run-command: name the cleanup function child_process_clear() run-command: export child_process_clear() daemon: plug memory leak Documentation/technical/api-run-command.txt | 7 +++++++ daemon.c | 1 + run-command.c | 12 ++++++------ run-command.h | 1 + 4 files changed, 15 insertions(+), 6 deletions(-) -- 2.6.2 ``` ## René Scharfe, 2015-10-31 09:19 Subject: [PATCH v2 1/3] run-command: name the cleanup function child_process_clear() Message-ID: <56348793.5040909@web.de> URL: https://gitlist.dev/e/56348793.5040909%40web.de In-Reply-To: <56348709.9080207@web.de> ``` Rename child_process_deinit() to child_process_clear() in order to stay consistent with similar cleanup functions like argv_array_clear(), string_list_clear() etc. Signed-off-by: Rene Scharfe --- run-command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/run-command.c b/run-command.c index 82cc238..b10ec75 100644 --- a/run-command.c +++ b/run-command.c @@ -13,7 +13,7 @@ void child_process_init(struct child_process *child) argv_array_init(&child->env_array); } -static void child_process_deinit(struct child_process *child) +static void child_process_clear(struct child_process *child) { argv_array_clear(&child->args); argv_array_clear(&child->env_array); @@ -335,7 +335,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)); - child_process_deinit(cmd); + child_process_clear(cmd); errno = failed_errno; return -1; } @@ -520,7 +520,7 @@ fail_pipe: close_pair(fderr); else if (cmd->err) close(cmd->err); - child_process_deinit(cmd); + child_process_clear(cmd); errno = failed_errno; return -1; } @@ -546,7 +546,7 @@ fail_pipe: int finish_command(struct child_process *cmd) { int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0); - child_process_deinit(cmd); + child_process_clear(cmd); return ret; } @@ -990,7 +990,7 @@ static void pp_cleanup(struct parallel_processes *pp) for (i = 0; i < pp->max_processes; i++) { strbuf_release(&pp->children[i].err); - child_process_deinit(&pp->children[i].process); + child_process_clear(&pp->children[i].process); } free(pp->children); @@ -1157,7 +1157,7 @@ static int pp_collect_finished(struct parallel_processes *pp) pp->nr_processes--; pp->children[i].in_use = 0; pp->pfd[i].fd = -1; - child_process_deinit(&pp->children[i].process); + child_process_clear(&pp->children[i].process); child_process_init(&pp->children[i].process); if (i != pp->output_owner) { -- 2.6.2 ``` ## René Scharfe, 2015-10-31 09:20 Subject: [PATCH v2 2/3] run-command: export child_process_clear() Message-ID: <563487C6.4050504@web.de> URL: https://gitlist.dev/e/563487C6.4050504%40web.de In-Reply-To: <56348709.9080207@web.de> ``` Make the API symmetric by including a cleanup function as a counterpart to child_process_init(). Signed-off-by: Rene Scharfe --- Documentation/technical/api-run-command.txt | 7 +++++++ run-command.c | 2 +- run-command.h | 1 + 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt index a9fdb45..8bf3e37 100644 --- a/Documentation/technical/api-run-command.txt +++ b/Documentation/technical/api-run-command.txt @@ -46,6 +46,13 @@ Functions The argument dir corresponds the member .dir. The argument env corresponds to the member .env. +`child_process_clear`:: + + Release the memory associated with the struct child_process. + Most users of the run-command API don't need to call this + function explicitly because `start_command` invokes it on + failure and `finish_command` calls it automatically already. + The functions above do the following: . If a system call failed, errno is set and -1 is returned. A diagnostic diff --git a/run-command.c b/run-command.c index b10ec75..0a3c24e 100644 --- a/run-command.c +++ b/run-command.c @@ -13,7 +13,7 @@ void child_process_init(struct child_process *child) argv_array_init(&child->env_array); } -static void child_process_clear(struct child_process *child) +void child_process_clear(struct child_process *child) { argv_array_clear(&child->args); argv_array_clear(&child->env_array); diff --git a/run-command.h b/run-command.h index 9fe37ee..e296bd2 100644 --- a/run-command.h +++ b/run-command.h @@ -47,6 +47,7 @@ struct child_process { #define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT, ARGV_ARRAY_INIT } void child_process_init(struct child_process *); +void child_process_clear(struct child_process *); int start_command(struct child_process *); int finish_command(struct child_process *); -- 2.6.2 ``` ## René Scharfe, 2015-10-31 09:20 Subject: [PATCH v2 3/3] daemon: plug memory leak Message-ID: <563487F4.6080403@web.de> URL: https://gitlist.dev/e/563487F4.6080403%40web.de In-Reply-To: <56348709.9080207@web.de> ``` Call child_process_clear() when a child ends to release the memory allocated for its environment. This is necessary because unlike all other users of start_command() we don't call finish_command(), which would have taken care of that for us. This leak was introduced by f063d38b (daemon: use cld->env_array when re-spawning). Signed-off-by: Rene Scharfe --- daemon.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon.c b/daemon.c index 56679a1..be70cd4 100644 --- a/daemon.c +++ b/daemon.c @@ -802,6 +802,7 @@ static void check_dead_children(void) /* remove the child */ *cradle = blanket->next; live_children--; + child_process_clear(&blanket->cld); free(blanket); } else cradle = &blanket->next; -- 2.6.2 ``` ## Jeff King, 2015-10-31 13:53 Subject: Re: [PATCH v2 0/3] daemon: plug memory leak Message-ID: <20151031135304.GA28310@sigill.intra.peff.net> URL: https://gitlist.dev/e/20151031135304.GA28310%40sigill.intra.peff.net In-Reply-To: <56348709.9080207@web.de> ``` On Sat, Oct 31, 2015 at 10:16:57AM +0100, René Scharfe wrote: > Changes since v1: > - Rebased on next, which already has a cleanup function. > - Added first patch for renaming it. > > Rene Scharfe (3): > run-command: name the cleanup function child_process_clear() > run-command: export child_process_clear() > daemon: plug memory leak This round looks good to me. Thanks. -Peff ``` ## Stefan Beller, 2015-10-31 20:32 Subject: Re: [PATCH v2 0/3] daemon: plug memory leak Message-ID: URL: https://gitlist.dev/e/CAGZ79kaXfkmSm0dn40kqrD95D2qpMo%2BNgEYbs%3DrVca5qnLTuMQ%40mail.gmail.com In-Reply-To: <20151031135304.GA28310@sigill.intra.peff.net> ``` On Sat, Oct 31, 2015 at 6:53 AM, Jeff King wrote: > On Sat, Oct 31, 2015 at 10:16:57AM +0100, René Scharfe wrote: > >> Changes since v1: >> - Rebased on next, which already has a cleanup function. >> - Added first patch for renaming it. >> >> Rene Scharfe (3): >> run-command: name the cleanup function child_process_clear() >> run-command: export child_process_clear() >> daemon: plug memory leak > > This round looks good to me. Thanks. Looks good to me, too Thanks Stefan > > -Peff ```