From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45908) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V0Vwu-00080E-HK for qemu-devel@nongnu.org; Sat, 20 Jul 2013 08:07:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V0Vws-00013M-Cn for qemu-devel@nongnu.org; Sat, 20 Jul 2013 08:07:44 -0400 Received: from cantor2.suse.de ([195.135.220.15]:47444 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V0Vwr-00013D-VE for qemu-devel@nongnu.org; Sat, 20 Jul 2013 08:07:42 -0400 Message-ID: <51EA7D88.1030707@suse.de> Date: Sat, 20 Jul 2013 14:07:36 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <21d3151db1cf00430db82f67ea6f66e93197e2c9.1373603020.git.peter.crosthwaite@xilinx.com> In-Reply-To: <21d3151db1cf00430db82f67ea6f66e93197e2c9.1373603020.git.peter.crosthwaite@xilinx.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC PATCH v1 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: peter.crosthwaite@xilinx.com Cc: Luiz Capitulino , peter.maydell@linaro.org, david@gibson.dropbear.id.au, qemu-devel@nongnu.org, agraf@suse.de Am 12.07.2013 06:29, schrieb peter.crosthwaite@xilinx.com: > From: Peter Crosthwaite >=20 > There are a mix of usages of the qemu_fdt_* API calls, some which "some of which" > wish to assert and abort QEMU on failure and some of which wish to do > their own error handling. The latter in more correct, but the former "is more" > is in the majority and more pragmatic, so facilitate both schemes by > creating asserting and non asserting variants. This patch does this > for qemu fdt_setprop and its wrapping users: >=20 > * qemu_fdt_setprop > * qemu_fdt_setprop_u64 > * qemu_fdt_setprop_cells >=20 > Error reporting is stylistically udpated to use Error ** instead of "updated" > integer return codes and exit(1). >=20 > Users of these APIs that ignore the return code are converted to using > the _assert variants. Users that check the return code are converted to > use Error ** for their error detection paths. >=20 > Signed-off-by: Peter Crosthwaite > --- > If this is ok, I will apply the change pattern to the entire > qemu_fdt API >=20 > device_tree.c | 35 ++++++++++++---- > hw/arm/boot.c | 7 ++-- > hw/ppc/e500.c | 66 +++++++++++++++--------------- > hw/ppc/e500plat.c | 6 +-- > hw/ppc/mpc8544ds.c | 6 +-- > hw/ppc/ppc440_bamboo.c | 8 ++-- > include/sysemu/device_tree.h | 97 ++++++++++++++++++++++++++++++++++++= +++++--- > 7 files changed, 166 insertions(+), 59 deletions(-) >=20 > diff --git a/device_tree.c b/device_tree.c > index 048b8e1..ca2a88d 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -4,8 +4,10 @@ > * interface. > * > * Copyright 2008 IBM Corporation. > + * Copyright 2013 Xilinx Inc. > * Authors: Jerone Young > * Hollis Blanchard > + * Peter Crosthwaite > * > * This work is licensed under the GNU GPL license version 2 or later. > * > @@ -126,19 +128,25 @@ static int findnode_nofail(void *fdt, const char = *node_path) > return offset; > } > =20 > -int qemu_fdt_setprop(void *fdt, const char *node_path, > - const char *property, const void *val, int size) > +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *pr= operty, > + const void *val, int size, Error **errp) > { > int r; > =20 > r =3D fdt_setprop(fdt, findnode_nofail(fdt, node_path), property, = val, size); > if (r < 0) { > - fprintf(stderr, "%s: Couldn't set %s/%s: %s\n", __func__, node= _path, > - property, fdt_strerror(r)); > - exit(1); > + error_setg(errp, "%s: Couldn't set %s/%s: %s\n", __func__, nod= e_path, > + property, fdt_strerror(r)); The error_set*() functions shouldn't get \n appended. I think it would be better to drop __func__, too. Otherwise the idea looks sane. Thanks for looking into this. > } > +} > =20 > - return r; > +void qemu_fdt_setprop_assert(void *fdt, const char *node_path, > + const char *property, const void *val, in= t size) > +{ > + Error *errp =3D NULL; Please use err for Error* here, errp is used for Error**. > + > + qemu_fdt_setprop(fdt, node_path, property, val, size, &errp); > + assert_no_error(errp); > } > =20 > int qemu_fdt_setprop_cell(void *fdt, const char *node_path, > @@ -156,11 +164,20 @@ int qemu_fdt_setprop_cell(void *fdt, const char *= node_path, > return r; > } > =20 > -int qemu_fdt_setprop_u64(void *fdt, const char *node_path, > - const char *property, uint64_t val) > +void qemu_fdt_setprop_u64(void *fdt, const char *node_path, > + const char *property, uint64_t val, Error **= errp) > { > val =3D cpu_to_be64(val); > - return qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val= )); > + qemu_fdt_setprop(fdt, node_path, property, &val, sizeof(val), errp= ); > +} > + > +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path, > + const char *property, uint64_t val) > +{ > + Error *errp =3D NULL; > + > + qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp); > + assert_no_error(errp); > } > =20 > int qemu_fdt_setprop_string(void *fdt, const char *node_path, [...] > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c > index db9d38b..39badfa 100644 > --- a/hw/ppc/ppc440_bamboo.c > +++ b/hw/ppc/ppc440_bamboo.c > @@ -64,6 +64,7 @@ static int bamboo_load_device_tree(hwaddr addr, > void *fdt; > uint32_t tb_freq =3D 400000000; > uint32_t clock_freq =3D 400000000; > + Error *errp =3D NULL; err > =20 > filename =3D qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TRE= E_FILE); > if (!filename) { > @@ -77,10 +78,11 @@ static int bamboo_load_device_tree(hwaddr addr, > =20 > /* Manipulate device tree in memory. */ > =20 > - ret =3D qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property, > - sizeof(mem_reg_property)); > - if (ret < 0) > + qemu_fdt_setprop(fdt, "/memory", "reg", mem_reg_property, > + sizeof(mem_reg_property), &errp); > + if (errp) { > fprintf(stderr, "couldn't set /memory/reg\n"); error_free(err) and possibly append it to the error message? > + } > =20 > ret =3D qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start"= , > initrd_base); > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.= h > index b4650c5..adaf5c2 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -4,8 +4,10 @@ > * interface. > * > * Copyright 2008 IBM Corporation. > + * Copyright 2013 Xilinx Inc. > * Authors: Jerone Young > * Hollis Blanchard > + * Peter Crosthwaite > * > * This work is licensed under the GNU GPL license version 2 or later. > * > @@ -14,15 +16,68 @@ > #ifndef __DEVICE_TREE_H__ > #define __DEVICE_TREE_H__ > =20 > +#include "qapi/qmp/qerror.h" qapi/error.h? > + > void *create_device_tree(int *sizep); > void *load_device_tree(const char *filename_path, int *sizep); > =20 > -int qemu_fdt_setprop(void *fdt, const char *node_path, > - const char *property, const void *val, int size); > +/** > + * qemu_fdt_setprop - create or change a property * qemu_fdt_setprop: Description goes below the arguments. > + * @fdt: pointer to the device tree blob > + * @node_path: node-path of the node whose property to change > + * @property: name of the property to change > + * @val: pointer to data to set the property value to > + * @size: length of the property value > + * @errp: Error to populate incase of error "in case" > + * > + * qemu_fdt_setprop() sets the value of the named property in the give= n > + * node to the given value and length, creating the property if it > + * does not already exist. > + */ > + > +void qemu_fdt_setprop(void *fdt, const char *node_path, const char *pr= operty, > + const void *val, int size, Error **errp); > + > +/** > + * qemu_fdt_setprop_assert - create or change a property and assert su= ccess > + * > + * Same as qemu_fdt_setprop() except no errp argument required, and > + * asserts the success of the operation. > + */ > + > +void qemu_fdt_setprop_assert(void *fdt, const char *node_path, > + const char *property, const void *val, in= t size); > + > int qemu_fdt_setprop_cell(void *fdt, const char *node_path, > const char *property, uint32_t val); > -int qemu_fdt_setprop_u64(void *fdt, const char *node_path, > - const char *property, uint64_t val); > + > +/** > + * qemu_fdt_setprop_u64 - create or change a 64bit int property > + * @fdt: pointer to the device tree blob > + * @node_path: node-path of the node whose property to change > + * @property: name of the property to change > + * @val: value to set > + * @errp: Error to populate incase of error > + * > + * qemu_fdt_setprop_u64() sets the value of the named property in the = given > + * node to the given uint64_t value. The value is converted to big end= ian > + * format as per device tree formatting. > + */ > + > +void qemu_fdt_setprop_u64(void *fdt, const char *node_path, > + const char *property, uint64_t val, Error **= errp); > + > +/** > + * qemu_fdt_setprop_u64_assert - create or change a 64 bit int propert= y and > + * assert success > + * > + * Same as qemu_fdt_setprop_u64() except no errp argument required, an= d > + * asserts the success of the operation. > + */ > + > +void qemu_fdt_setprop_u64_assert(void *fdt, const char *node_path, > + const char *property, uint64_t val); > + > int qemu_fdt_setprop_string(void *fdt, const char *node_path, > const char *property, const char *string); > int qemu_fdt_setprop_phandle(void *fdt, const char *node_path, > @@ -37,7 +92,21 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt); > int qemu_fdt_nop_node(void *fdt, const char *node_path); > int qemu_fdt_add_subnode(void *fdt, const char *name); > =20 > -#define qemu_fdt_setprop_cells(fdt, node_path, property, ...) = \ > +/** > + * qemu_fdt_setprop_cells - create or change a multi-cell property > + * @fdt: pointer to the device tree blob > + * @node_path: node-path of the node whose property to change > + * @property: name of the property to change > + * @errp: Error to populate incase of error > + * @...: varargs list of cells to write to property > + * > + * qemu_fdt_setprop_cells() sets the value of the named property in th= e given > + * node to a list of cells. The varargs are converted to an appropriat= e length > + * uint32_t array and converted to big endian. The array is then writt= en as > + * the property value. > + */ > + > +#define qemu_fdt_setprop_cells(fdt, node_path, property, errp, ...) = \ > do { = \ > uint32_t qdt_tmp[] =3D { __VA_ARGS__ }; = \ > int i; = \ > @@ -46,7 +115,23 @@ int qemu_fdt_add_subnode(void *fdt, const char *nam= e); > qdt_tmp[i] =3D cpu_to_be32(qdt_tmp[i]); = \ > } = \ > qemu_fdt_setprop(fdt, node_path, property, qdt_tmp, = \ > - sizeof(qdt_tmp)); = \ > + sizeof(qdt_tmp), errp); = \ > + } while (0) > + > +/** > + * qemu_fdt_setprop_cells_assert - create or change a mult-cell proper= ty and > + * assert success > + * > + * Same as qemu_fdt_setprop_cells() except no errp argument required, = and > + * asserts the success of the operation. > + */ > + > +#define qemu_fdt_setprop_cells_assert(fdt, node_path, property, ...) = \ > + do { = \ > + Error *errp =3D NULL; = \ err > + = \ > + qemu_fdt_setprop_cells(fdt, node_path, property, &errp, __VA_A= RGS__); \ > + assert_no_error(errp); = \ > } while (0) > =20 > void qemu_fdt_dumpdtb(void *fdt, int size); Regards, Andreas --=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