| Message ID | 1452245525-3886-6-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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.auger@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. Otherwise: Reviewed-by: Peter Crosthwaite <crosthwaite.peter@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 >
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.auger@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.peter@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 >>
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) + */ 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);
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.auger@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(-)