Re: [Qemu-devel] [RFC PATCH v1 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting

["Andreas Färber" <afaerber@suse.de>]

Am 12.07.2013 06:29, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> 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:
> 
>  * qemu_fdt_setprop
>  * qemu_fdt_setprop_u64
>  * qemu_fdt_setprop_cells
> 
> Error reporting is stylistically udpated to use Error ** instead of

"updated"

> integer return codes and exit(1).
> 
> 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.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> If this is ok, I will apply the change pattern to the entire
> qemu_fdt API
> 
>  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(-)
> 
> 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 <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * 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;
>  }
>  
> -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 *property,
> +                      const void *val, int size, Error **errp)
>  {
>      int r;
>  
>      r = 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__, node_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.

>      }
> +}
>  
> -    return r;
> +void qemu_fdt_setprop_assert(void *fdt, const char *node_path,
> +                             const char *property, const void *val, int size)
> +{
> +    Error *errp = 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);
>  }
>  
>  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;
>  }
>  
> -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 = 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 = NULL;
> +
> +    qemu_fdt_setprop_u64(fdt, node_path, property, val, &errp);
> +    assert_no_error(errp);
>  }
>  
>  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 = 400000000;
>      uint32_t clock_freq = 400000000;
> +    Error *errp = NULL;

err

>  
>      filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, BINARY_DEVICE_TREE_FILE);
>      if (!filename) {
> @@ -77,10 +78,11 @@ static int bamboo_load_device_tree(hwaddr addr,
>  
>      /* Manipulate device tree in memory. */
>  
> -    ret = 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?

> +    }
>  
>      ret = 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 <jyoung5@us.ibm.com>
>   *          Hollis Blanchard <hollisb@us.ibm.com>
> + *          Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>   *
>   * 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__
>  
> +#include "qapi/qmp/qerror.h"

qapi/error.h?

> +
>  void *create_device_tree(int *sizep);
>  void *load_device_tree(const char *filename_path, int *sizep);
>  
> -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 given
> + * 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 *property,
> +                      const void *val, int size, Error **errp);
> +
> +/**
> + * qemu_fdt_setprop_assert - create or change a property and assert success
> + *
> + * 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, int 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 endian
> + * 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 property and
> + * assert success
> + *
> + * Same as qemu_fdt_setprop_u64() except no errp argument required, and
> + * 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);
>  
> -#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 the given
> + * node to a list of cells. The varargs are converted to an appropriate length
> + * uint32_t array and converted to big endian. The array is then written as
> + * the property value.
> + */
> +
> +#define qemu_fdt_setprop_cells(fdt, node_path, property, errp, ...)           \
>      do {                                                                      \
>          uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
>          int i;                                                                \
> @@ -46,7 +115,23 @@ int qemu_fdt_add_subnode(void *fdt, const char *name);
>              qdt_tmp[i] = 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 property 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 = NULL;                                                   \

err

> +                                                                              \
> +        qemu_fdt_setprop_cells(fdt, node_path, property, &errp, __VA_ARGS__); \
> +        assert_no_error(errp);                                                \
>      } while (0)
>  
>  void qemu_fdt_dumpdtb(void *fdt, int size);

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg