Hi Peter, On 01/12/2016 04:58 AM, Peter Crosthwaite wrote: > On Fri, Jan 08, 2016 at 09:32:02AM +0000, Eric Auger wrote: >> This patch aligns the prototype with qemu_fdt_getprop. The caller >> can choose whether the function self-asserts on error (passing >> &error_fatal as Error ** argument, corresponding to the legacy behavior), >> or behaves differently such as simply output a message. >> >> In this later case the caller can use the new lenp parameter to interpret >> the error if any. >> >> Signed-off-by: Eric Auger <eric.au...@linaro.org> >> >> --- >> >> v3 : creation >> --- >> device_tree.c | 21 ++++++++++++++------- >> hw/arm/boot.c | 6 ++++-- >> hw/arm/vexpress.c | 6 ++++-- >> include/sysemu/device_tree.h | 16 +++++++++++++++- >> 4 files changed, 37 insertions(+), 12 deletions(-) >> >> diff --git a/device_tree.c b/device_tree.c >> index 6ecc9da..0e837bf 100644 >> --- a/device_tree.c >> +++ b/device_tree.c >> @@ -338,15 +338,22 @@ const void *qemu_fdt_getprop(void *fdt, const char >> *node_path, >> } >> >> uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, >> - const char *property) >> + const char *property, int *lenp, Error >> **errp) >> { >> int len; >> - const uint32_t *p = qemu_fdt_getprop(fdt, node_path, property, &len, >> - &error_fatal); >> - if (len != 4) { >> - error_report("%s: %s/%s not 4 bytes long (not a cell?)", >> - __func__, node_path, property); >> - exit(1); >> + const uint32_t *p; >> + >> + if (!lenp) { >> + lenp = &len; >> + } >> + p = qemu_fdt_getprop(fdt, node_path, property, lenp, errp); >> + if (!p) { >> + return 0; >> + } else if (*lenp != 4) { >> + error_setg(errp, "%s: %s/%s not 4 bytes long (not a cell?)", >> + __func__, node_path, property); >> + *lenp = -EINVAL; >> + return 0; >> } >> return be32_to_cpu(*p); >> } >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index 75f69bf..541b74c 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -386,8 +386,10 @@ static int load_dtb(hwaddr addr, const struct >> arm_boot_info *binfo, >> return 0; >> } >> >> - acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >> - scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >> + acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells", >> + NULL, &error_fatal); >> + scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells", >> + NULL, &error_fatal); >> if (acells == 0 || scells == 0) { >> fprintf(stderr, "dtb file invalid (#address-cells or #size-cells >> 0)\n"); >> goto fail; >> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c >> index 058abbd..ffe42be 100644 >> --- a/hw/arm/vexpress.c >> +++ b/hw/arm/vexpress.c >> @@ -482,8 +482,10 @@ static void vexpress_modify_dtb(const struct >> arm_boot_info *info, void *fdt) >> uint32_t acells, scells, intc; >> const VEDBoardInfo *daughterboard = (const VEDBoardInfo *)info; >> >> - acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells"); >> - scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells"); >> + acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells", >> + NULL, &error_fatal); >> + scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells", >> + NULL, &error_fatal); >> intc = find_int_controller(fdt); >> if (!intc) { >> /* Not fatal, we just won't provide virtio. This will >> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h >> index 4d7cbb9..5aa750b 100644 >> --- a/include/sysemu/device_tree.h >> +++ b/include/sysemu/device_tree.h >> @@ -60,8 +60,22 @@ int qemu_fdt_setprop_phandle(void *fdt, const char >> *node_path, >> const void *qemu_fdt_getprop(void *fdt, const char *node_path, >> const char *property, int *lenp, >> Error **errp); >> +/** >> + * qemu_fdt_getprop_cell: retrieve the value of a given 4 byte property >> + * @fdt: pointer to the device tree blob >> + * @node_path: node path >> + * @property: name of the property to find >> + * @lenp: fdt error if any or -EINVAL if the property size is different from >> + * 4 bytes, or 4 (expected length of the property) upon success. >> + * @errp: handle to an error object >> + * >> + * returns the property value on success >> + * in case errp is set to &error_fatal, the function auto-asserts >> + * on error (legacy behavior) > > Avoid re-documenting the semantics of the Error API, as this comment > may silently go stale with a patch to the Error API. The function now simply > accepts an Error ** and that implies the behaviour of error_fatal. With very > few callers there is not much of a legacy to document and it is not a user > visible legacy anyway. OK I will remove that comment. Thanks for the reviews and R-b's
Best Regards Eric > > Otherwise: > > Reviewed-by: Peter Crosthwaite <crosthwaite.pe...@gmail.com> > > Regards, > Peter > >> + */ >> uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, >> - const char *property); >> + const char *property, int *lenp, >> + Error **errp); >> uint32_t qemu_fdt_get_phandle(void *fdt, const char *path); >> uint32_t qemu_fdt_alloc_phandle(void *fdt); >> int qemu_fdt_nop_node(void *fdt, const char *node_path); >> -- >> 1.9.1 >>