On Thu, 29 Jul 2021 08:08:36 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> wrote:
> When an input hanlder to a trace file is closed with tracecmd_close(),
> the list with buffers is not freed. This leads to a memory leak. Added
> logic to free that list.
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@xxxxxxxxx>
> ---
> lib/trace-cmd/trace-input.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index 9253bc37..af11cbc6 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -3483,7 +3483,7 @@ void tracecmd_ref(struct tracecmd_input *handle)
> */
> void tracecmd_close(struct tracecmd_input *handle)
> {
> - int cpu;
> + int i;
It's OK to add int i. There's no reason to rename cpu, as I believe the
"cpu" variable is still appropriate. The compiled code will have no
difference, as its a simple optimization to merge multiple local variables.
>
> if (!handle)
> return;
> @@ -3496,21 +3496,21 @@ void tracecmd_close(struct tracecmd_input *handle)
> if (--handle->ref)
> return;
>
> - for (cpu = 0; cpu < handle->cpus; cpu++) {
> + for (i = 0; i < handle->cpus; i++) {
> /* The tracecmd_peek_data may have cached a record */
> - free_next(handle, cpu);
> - free_page(handle, cpu);
> - if (handle->cpu_data && handle->cpu_data[cpu].kbuf) {
> - kbuffer_free(handle->cpu_data[cpu].kbuf);
> - if (handle->cpu_data[cpu].page_map)
> - free_page_map(handle->cpu_data[cpu].page_map);
> -
> - if (handle->cpu_data[cpu].page_cnt)
> + free_next(handle, i);
> + free_page(handle, i);
> + if (handle->cpu_data && handle->cpu_data[i].kbuf) {
> + kbuffer_free(handle->cpu_data[i].kbuf);
> + if (handle->cpu_data[i].page_map)
> + free_page_map(handle->cpu_data[i].page_map);
> +
> + if (handle->cpu_data[i].page_cnt)
> tracecmd_warning("%d pages still allocated on cpu %d%s",
> - handle->cpu_data[cpu].page_cnt, cpu,
> - show_records(handle->cpu_data[cpu].pages,
> - handle->cpu_data[cpu].nr_pages));
> - free(handle->cpu_data[cpu].pages);
> + handle->cpu_data[i].page_cnt, i,
> + show_records(handle->cpu_data[i].pages,
> + handle->cpu_data[i].nr_pages));
> + free(handle->cpu_data[i].pages);
One reason not to change it, is because I'm staring at this code trying
to figure out what logical change happened here. Then I realize, that
it's just a variable change.
Let's just add "int i" and keep "int cpu". Especially since I plan on
backporting this patch, and I don't need unnecessary conflicts to deal
with.
> }
> }
>
> @@ -3521,6 +3521,11 @@ void tracecmd_close(struct tracecmd_input *handle)
> free(handle->version);
> close(handle->fd);
>
> + for (i = 0; i < handle->nr_buffers; i++)
> + free(handle->buffers[i].name);
> +
> + free(handle->buffers);
I take it, this patch only does the above.
-- Steve
> +
> tracecmd_free_hooks(handle->hooks);
> handle->hooks = NULL;
>
|