[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v1 3/3] device_tree: qemu_fdt_setprop: Fixup
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 3/3] device_tree: qemu_fdt_setprop: Fixup error reporting |
Date: |
Sat, 20 Jul 2013 14:07:36 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130620 Thunderbird/17.0.7 |
Am 12.07.2013 06:29, schrieb address@hidden:
> From: Peter Crosthwaite <address@hidden>
>
> 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 <address@hidden>
> ---
> 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 <address@hidden>
> * Hollis Blanchard <address@hidden>
> + * Peter Crosthwaite <address@hidden>
> *
> * 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 <address@hidden>
> * Hollis Blanchard <address@hidden>
> + * Peter Crosthwaite <address@hidden>
> *
> * 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