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

[Andreas Färber]

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 ?

> ---
>  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 <address@hidden>

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


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg