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

[Eric Auger]

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 <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.
OK I will remove that comment. Thanks for the reviews and R-b's

Best Regards

Eric
> 
> 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
>>