Re: [Qemu-devel] [PATCH] arm/boot: Free dtb blob memory after use

[Peter Maydell]

On 14 June 2013 18:14, Andreas Färber <address@hidden> wrote:
> Am 14.06.2013 13:27, schrieb Peter Maydell:
>> The dtb blob returned by load_device_tree() is in memory allocated
>> with g_malloc(). Free it accordingly once we have copied its
>> contents into the guest memory. To make this easy, we need also to
>> clean up the error handling in load_dtb() so that we consistently
>> handle errors in the same way (by printing a message and then
>> returning -1, rather than either plowing on or exiting immediately).
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>
> Cc: address@hidden ?

It's only a once-per-run leak of a blob that's going to be <10K
in size, so it doesn't really seem worth worrying about for stable.

>> ---
>>  hw/arm/boot.c |   20 +++++++++++++++-----
>>  1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index f8c2031..f5870f6 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -238,14 +238,14 @@ static int load_dtb(hwaddr addr, const struct 
>> arm_boot_info *binfo)
>>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
>>      if (!filename) {
>>          fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_filename);
>> -        return -1;
>> +        goto fail;
>
> fdt is NULL-initialized, so this is OK.
>
>>      }
>>
>>      fdt = load_device_tree(filename, &size);
>>      if (!fdt) {
>>          fprintf(stderr, "Couldn't open dtb file %s\n", filename);
>>          g_free(filename);
>> -        return -1;
>> +        goto fail;
>>      }
>>      g_free(filename);
>>
>
> Personally I would've left those two unchanged for clarity, but your
> call. ;)

I liked the consistency of having every error path go the same way.

> arm_load_kernel() does exit(1) on failure, so only functional change is
> not ignoring failure to set memory, cmdline and initrd properties.
>
> Rest looks okay, except that I wonder whether we might later want to
> propagate these errors up the call chain and therefore use error_setg()
> and void here and a more central fprintf() in arm_load_kernel() - we
> surely don't want to duplicate it into each board even though there
> being four other fprintf()s in arm_load_kernel().

Mmm. Certainly now the error functions can handle ad-hoc
strings this is more attractive than when the code was first
written. I'm not sure it's worthwhile unless we want to move
in the direction of having all board init errors passed up
to the top level vl.c code, though.

> But that's orthogonal to making fdt errors fatal and the fdt cleanup, so
>
> Reviewed-by: Andreas Färber <address@hidden>

Thanks.

-- PMM