[Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Qemu-devel] [PATCH v2] microblaze: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

When not assign a -dtb argument, the variable dtb_filename
storage returned from qemu_find_file(), which should be freed
after use. Alternatively we define a local variable filename,
with 'char *' type, free after use.

Cc: Michael Tokarev <mjt@tls.msk.ru>
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
v2: fix a complier error.
---
 hw/microblaze/boot.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index a2843cd..352ccb1 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -113,15 +113,15 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *dtb_arg;
+    char *filename = NULL;
 
     machine_opts = qemu_get_machine_opts();
     kernel_filename = qemu_opt_get(machine_opts, "kernel");
     kernel_cmdline = qemu_opt_get(machine_opts, "append");
     dtb_arg = qemu_opt_get(machine_opts, "dtb");
-    if (dtb_arg) { /* Preference a -dtb argument */
-        dtb_filename = dtb_arg;
-    } else { /* default to pcbios dtb as passed by machine_init */
-        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
+    /* default to pcbios dtb as passed by machine_init */
+    if (!dtb_arg) {
+        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
     }
 
     boot_info.machine_cpu_reset = machine_cpu_reset;
@@ -203,7 +203,8 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
                             boot_info.initrd_start,
                             boot_info.initrd_end,
                             kernel_cmdline,
-                            dtb_filename);
+                            /* Preference a -dtb argument */
+                            dtb_arg ? dtb_arg : filename);
     }
-
+    g_free(filename);
 }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Gonglei]

On 2015/3/5 11:05, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> When not assign a -dtb argument, the variable dtb_filename
> storage returned from qemu_find_file(), which should be freed
> after use. Alternatively we define a local variable filename,
> with 'char *' type, free after use.
> 
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> v2: fix a complier error.
> ---
>  hw/microblaze/boot.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 

Ping...

> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index a2843cd..352ccb1 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -113,15 +113,15 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *dtb_arg;
> +    char *filename = NULL;
>  
>      machine_opts = qemu_get_machine_opts();
>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
>      kernel_cmdline = qemu_opt_get(machine_opts, "append");
>      dtb_arg = qemu_opt_get(machine_opts, "dtb");
> -    if (dtb_arg) { /* Preference a -dtb argument */
> -        dtb_filename = dtb_arg;
> -    } else { /* default to pcbios dtb as passed by machine_init */
> -        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> +    /* default to pcbios dtb as passed by machine_init */
> +    if (!dtb_arg) {
> +        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
>      }
>  
>      boot_info.machine_cpu_reset = machine_cpu_reset;
> @@ -203,7 +203,8 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>                              boot_info.initrd_start,
>                              boot_info.initrd_end,
>                              kernel_cmdline,
> -                            dtb_filename);
> +                            /* Preference a -dtb argument */
> +                            dtb_arg ? dtb_arg : filename);
>      }
> -
> +    g_free(filename);
>  }
> 

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Gonglei]

Hi,

   Ping again...  Can this patch be a raw stuff for rc2 ?

Regards,
Gonglei

On 2015/3/17 15:15, Gonglei wrote:
> On 2015/3/5 11:05, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> When not assign a -dtb argument, the variable dtb_filename
>> storage returned from qemu_find_file(), which should be freed
>> after use. Alternatively we define a local variable filename,
>> with 'char *' type, free after use.
>>
>> Cc: Michael Tokarev <mjt@tls.msk.ru>
>> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>> v2: fix a complier error.
>> ---
>>  hw/microblaze/boot.c | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
> 
> Ping...
> 
>> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
>> index a2843cd..352ccb1 100644
>> --- a/hw/microblaze/boot.c
>> +++ b/hw/microblaze/boot.c
>> @@ -113,15 +113,15 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>>      const char *kernel_filename;
>>      const char *kernel_cmdline;
>>      const char *dtb_arg;
>> +    char *filename = NULL;
>>  
>>      machine_opts = qemu_get_machine_opts();
>>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
>>      kernel_cmdline = qemu_opt_get(machine_opts, "append");
>>      dtb_arg = qemu_opt_get(machine_opts, "dtb");
>> -    if (dtb_arg) { /* Preference a -dtb argument */
>> -        dtb_filename = dtb_arg;
>> -    } else { /* default to pcbios dtb as passed by machine_init */
>> -        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
>> +    /* default to pcbios dtb as passed by machine_init */
>> +    if (!dtb_arg) {
>> +        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
>>      }
>>  
>>      boot_info.machine_cpu_reset = machine_cpu_reset;
>> @@ -203,7 +203,8 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>>                              boot_info.initrd_start,
>>                              boot_info.initrd_end,
>>                              kernel_cmdline,
>> -                            dtb_filename);
>> +                            /* Preference a -dtb argument */
>> +                            dtb_arg ? dtb_arg : filename);
>>      }
>> -
>> +    g_free(filename);
>>  }
>>
> 

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 2634 bytes --]

On 28/03/2015 12:38 pm, "Gonglei" <arei.gonglei@huawei.com> wrote:
>
> Hi,
>
>    Ping again...  Can this patch be a raw stuff for rc2 ?

Looks good to me, thanks.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Can this go through -trivial?

Cheers,
Edgar

>
> Regards,
> Gonglei
>
> On 2015/3/17 15:15, Gonglei wrote:
> > On 2015/3/5 11:05, arei.gonglei@huawei.com wrote:
> >> From: Gonglei <arei.gonglei@huawei.com>
> >>
> >> When not assign a -dtb argument, the variable dtb_filename
> >> storage returned from qemu_find_file(), which should be freed
> >> after use. Alternatively we define a local variable filename,
> >> with 'char *' type, free after use.
> >>
> >> Cc: Michael Tokarev <mjt@tls.msk.ru>
> >> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> ---
> >> v2: fix a complier error.
> >> ---
> >>  hw/microblaze/boot.c | 13 +++++++------
> >>  1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >
> > Ping...
> >
> >> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> >> index a2843cd..352ccb1 100644
> >> --- a/hw/microblaze/boot.c
> >> +++ b/hw/microblaze/boot.c
> >> @@ -113,15 +113,15 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu,
hwaddr ddr_base,
> >>      const char *kernel_filename;
> >>      const char *kernel_cmdline;
> >>      const char *dtb_arg;
> >> +    char *filename = NULL;
> >>
> >>      machine_opts = qemu_get_machine_opts();
> >>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
> >>      kernel_cmdline = qemu_opt_get(machine_opts, "append");
> >>      dtb_arg = qemu_opt_get(machine_opts, "dtb");
> >> -    if (dtb_arg) { /* Preference a -dtb argument */
> >> -        dtb_filename = dtb_arg;
> >> -    } else { /* default to pcbios dtb as passed by machine_init */
> >> -        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
dtb_filename);
> >> +    /* default to pcbios dtb as passed by machine_init */
> >> +    if (!dtb_arg) {
> >> +        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> >>      }
> >>
> >>      boot_info.machine_cpu_reset = machine_cpu_reset;
> >> @@ -203,7 +203,8 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu,
hwaddr ddr_base,
> >>                              boot_info.initrd_start,
> >>                              boot_info.initrd_end,
> >>                              kernel_cmdline,
> >> -                            dtb_filename);
> >> +                            /* Preference a -dtb argument */
> >> +                            dtb_arg ? dtb_arg : filename);
> >>      }
> >> -
> >> +    g_free(filename);
> >>  }
> >>
> >
>
>

[-- Attachment #2: Type: text/html, Size: 4154 bytes --]

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Gonglei]

On 2015/3/28 15:31, Edgar E. Iglesias wrote:
> On 28/03/2015 12:38 pm, "Gonglei" <arei.gonglei@huawei.com> wrote:
>>
>> Hi,
>>
>>    Ping again...  Can this patch be a raw stuff for rc2 ?
> 
> Looks good to me, thanks.
> 
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
Thanks :)

> Can this go through -trivial?
>
It's ok, but I don't know if -trivial branch maintainer has a plan to send a pull request
for rc2.

Michael?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Michael Tokarev]

28.03.2015 10:46, Gonglei wrote:
[]
>> Can this go through -trivial?
>>
> It's ok, but I don't know if -trivial branch maintainer has a plan to send a pull request
> for rc2.

I do have plan to send a pull request, but this thing is ugly.  The
original code is ugly, and your patch does not make it better.  I
wanted to make some changes myself but didn't find fime for that.

Ofcourse it is okay to apply your change to plug the leak, but..
oh well... :(  And if we do, we'll sure forget about that completely.
Now at least you're pinging me from time to time... ;))

Thanks,

/mjt

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Gonglei]

On 2015/3/30 1:09, Michael Tokarev wrote:
> 28.03.2015 10:46, Gonglei wrote:
> []
>>> Can this go through -trivial?
>>>
>> It's ok, but I don't know if -trivial branch maintainer has a plan to send a pull request
>> for rc2.
> 
> I do have plan to send a pull request, but this thing is ugly.  The
> original code is ugly, and your patch does not make it better.  I
> wanted to make some changes myself but didn't find fime for that.
> 
> Ofcourse it is okay to apply your change to plug the leak, but..
> oh well... :(  And if we do, we'll sure forget about that completely.
> Now at least you're pinging me from time to time... ;))
> 

Maybe you are waiting for my ping, right?  :)
Do you have any time for this leak or code optimization now?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Max Filippov]

On Thu, Mar 5, 2015 at 6:05 AM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> When not assign a -dtb argument, the variable dtb_filename
> storage returned from qemu_find_file(), which should be freed
> after use. Alternatively we define a local variable filename,
> with 'char *' type, free after use.
>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
> v2: fix a complier error.
> ---
>  hw/microblaze/boot.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Peter Crosthwaite]

On Wed, Mar 4, 2015 at 7:05 PM,  <arei.gonglei@huawei.com> wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> When not assign a -dtb argument, the variable dtb_filename
> storage returned from qemu_find_file(), which should be freed
> after use. Alternatively we define a local variable filename,
> with 'char *' type, free after use.
>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
> v2: fix a complier error.
> ---
>  hw/microblaze/boot.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index a2843cd..352ccb1 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -113,15 +113,15 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>      const char *kernel_filename;
>      const char *kernel_cmdline;
>      const char *dtb_arg;
> +    char *filename = NULL;
>
>      machine_opts = qemu_get_machine_opts();
>      kernel_filename = qemu_opt_get(machine_opts, "kernel");
>      kernel_cmdline = qemu_opt_get(machine_opts, "append");
>      dtb_arg = qemu_opt_get(machine_opts, "dtb");
> -    if (dtb_arg) { /* Preference a -dtb argument */
> -        dtb_filename = dtb_arg;
> -    } else { /* default to pcbios dtb as passed by machine_init */
> -        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> +    /* default to pcbios dtb as passed by machine_init */
> +    if (!dtb_arg) {
> +        filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
>      }
>
>      boot_info.machine_cpu_reset = machine_cpu_reset;
> @@ -203,7 +203,8 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>                              boot_info.initrd_start,
>                              boot_info.initrd_end,
>                              kernel_cmdline,
> -                            dtb_filename);
> +                            /* Preference a -dtb argument */
> +                            dtb_arg ? dtb_arg : filename);
>      }
> -
> +    g_free(filename);
>  }
> --
> 1.7.12.4
>
>
>

Re: [Qemu-devel] [PATCH v2] microblaze: fix memory leak

[Michael Tokarev]

05.03.2015 06:05, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> When not assign a -dtb argument, the variable dtb_filename
> storage returned from qemu_find_file(), which should be freed
> after use. Alternatively we define a local variable filename,
> with 'char *' type, free after use.

Actually now when I look at it all again I think this is the
solution I was thinking about when saying about a "better fix".
I dunno why I haven't seen this before.

I just applied this patch to -trivial. Thank you very much for
your patience!

/mjt