arm/boot: Free dtb blob memory after use

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 <peter.maydell@linaro.org>
---
 hw/arm/boot.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

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 <peter.maydell@linaro.org>

Cc: qemu-stable@nongnu.org ?

> ---
>  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. ;)

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().
But that's orthogonal to making fdt errors fatal and the fdt cleanup, so

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas


> @@ -253,7 +253,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>      scells = qemu_devtree_getprop_cell(fdt, "/", "#size-cells");
>      if (acells == 0 || scells == 0) {
>          fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> -        return -1;
> +        goto fail;
>      }
>  
>      mem_reg_propsize = acells + scells;
> @@ -265,7 +265,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>      } else if (hival != 0) {
>          fprintf(stderr, "qemu: dtb file not compatible with "
>                  "RAM start address > 4GB\n");
> -        exit(1);
> +        goto fail;
>      }
>      mem_reg_property[acells + scells - 1] = cpu_to_be32(binfo->ram_size);
>      hival = cpu_to_be32(binfo->ram_size >> 32);
> @@ -274,13 +274,14 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>      } else if (hival != 0) {
>          fprintf(stderr, "qemu: dtb file not compatible with "
>                  "RAM size > 4GB\n");
> -        exit(1);
> +        goto fail;
>      }
>  
>      rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
>                                mem_reg_propsize * sizeof(uint32_t));
>      if (rc < 0) {
>          fprintf(stderr, "couldn't set /memory/reg\n");
> +        goto fail;
>      }
>  
>      if (binfo->kernel_cmdline && *binfo->kernel_cmdline) {
> @@ -288,6 +289,7 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>                                            binfo->kernel_cmdline);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +            goto fail;
>          }
>      }
>  
> @@ -296,20 +298,28 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
>                  binfo->initrd_start);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-start\n");
> +            goto fail;
>          }
>  
>          rc = qemu_devtree_setprop_cell(fdt, "/chosen", "linux,initrd-end",
>                      binfo->initrd_start + binfo->initrd_size);
>          if (rc < 0) {
>              fprintf(stderr, "couldn't set /chosen/linux,initrd-end\n");
> +            goto fail;
>          }
>      }
>      qemu_devtree_dumpdtb(fdt, size);
>  
>      cpu_physical_memory_write(addr, fdt, size);
>  
> +    g_free(fdt);
> +
>      return 0;
>  
> +fail:
> +    g_free(fdt);
> +    return -1;
> +
>  #else
>      fprintf(stderr, "Device tree requested, "
>                  "but qemu was compiled without fdt support\n");
>

On 14 June 2013 18:14, Andreas Färber <afaerber@suse.de> 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 <peter.maydell@linaro.org>
>
> Cc: qemu-stable@nongnu.org ?

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 <afaerber@suse.de>

Thanks.

-- PMM

On 14 June 2013 12:27, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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 <peter.maydell@linaro.org>

Applied to arm-devs.next (fixing up the trivial conflict with
the "drop CONFIG_FDT" patch in the process).

-- PMM