From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UnXZf-0008Ny-3o for qemu-devel@nongnu.org; Fri, 14 Jun 2013 13:14:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UnXZa-0005bu-VK for qemu-devel@nongnu.org; Fri, 14 Jun 2013 13:14:07 -0400 Message-ID: <51BB4F59.5050608@suse.de> Date: Fri, 14 Jun 2013 19:14:01 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1371209256-11408-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1371209256-11408-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] arm/boot: Free dtb blob memory after use List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-stable , qemu-devel@nongnu.org, patches@linaro.org 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). >=20 > Signed-off-by: Peter Maydell Cc: qemu-stable@nongnu.org ? > --- > hw/arm/boot.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) >=20 > 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 =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filena= me); > if (!filename) { > fprintf(stderr, "Couldn't open dtb file %s\n", binfo->dtb_file= name); > - return -1; > + goto fail; fdt is NULL-initialized, so this is OK. > } > =20 > fdt =3D 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); > =20 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=E4rber Andreas > @@ -253,7 +253,7 @@ static int load_dtb(hwaddr addr, const struct arm_b= oot_info *binfo) > scells =3D qemu_devtree_getprop_cell(fdt, "/", "#size-cells"); > if (acells =3D=3D 0 || scells =3D=3D 0) { > fprintf(stderr, "dtb file invalid (#address-cells or #size-cel= ls 0)\n"); > - return -1; > + goto fail; > } > =20 > mem_reg_propsize =3D acells + scells; > @@ -265,7 +265,7 @@ static int load_dtb(hwaddr addr, const struct arm_b= oot_info *binfo) > } else if (hival !=3D 0) { > fprintf(stderr, "qemu: dtb file not compatible with " > "RAM start address > 4GB\n"); > - exit(1); > + goto fail; > } > mem_reg_property[acells + scells - 1] =3D cpu_to_be32(binfo->ram_s= ize); > hival =3D 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 !=3D 0) { > fprintf(stderr, "qemu: dtb file not compatible with " > "RAM size > 4GB\n"); > - exit(1); > + goto fail; > } > =20 > rc =3D qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_propert= y, > mem_reg_propsize * sizeof(uint32_t)); > if (rc < 0) { > fprintf(stderr, "couldn't set /memory/reg\n"); > + goto fail; > } > =20 > if (binfo->kernel_cmdline && *binfo->kernel_cmdline) { > @@ -288,6 +289,7 @@ static int load_dtb(hwaddr addr, const struct arm_b= oot_info *binfo) > binfo->kernel_cmdline); > if (rc < 0) { > fprintf(stderr, "couldn't set /chosen/bootargs\n"); > + goto fail; > } > } > =20 > @@ -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; > } > =20 > rc =3D 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); > =20 > cpu_physical_memory_write(addr, fdt, size); > =20 > + g_free(fdt); > + > return 0; > =20 > +fail: > + g_free(fdt); > + return -1; > + > #else > fprintf(stderr, "Device tree requested, " > "but qemu was compiled without fdt support\n"); >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg