Re: [Qemu-devel] [PATCH v4 5/8] device_tree: qemu_fdt_getprop_cell converted to use the error API

[Peter Crosthwaite]

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

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
>