[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 10/15] qbus_find_recursive(): push Error and
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 10/15] qbus_find_recursive(): push Error and make it terminate a recursive search |
Date: |
Wed, 20 Mar 2013 16:13:52 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Laszlo Ersek <address@hidden> writes:
> Push error handling to callers.
>
> When a bus is found by name and it has no free slots, that's a fatal error
> for the recursive search. Clarify the interface contract, and use error
> propagation to unwind the recursion quickly.
>
> Conversion status (call chains covered or substituted by error propagation
> marked with square brackets):
>
> do_device_add -> [qemu_find_opts -> error_report]
> do_device_add -> qdev_device_add -> qerror_report
> do_device_add -> qdev_device_add -> qbus_find -> [qbus_find_recursive
> -> qerror_report]
> do_device_add -> qdev_device_add -> qbus_find -> qerror_report
> do_device_add -> qdev_device_add -> [set_property -> qdev_prop_parse
> -> qerror_report_err]
>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> hw/qdev-monitor.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 0a0bced..4fa64c9 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -288,6 +288,40 @@ static DeviceState *qbus_find_dev(BusState *bus, char
> *elem)
> return NULL;
> }
>
> +/*
> + * qbus_find_recursive() -- locate a bus by name and/or type in a subtree
> + *
> + * Search the bus subtree rooted in @bus for a bus matching the optionally
> + * specified @name and the optionally specified @type attributes. Matching
> + * candidates are examined for a free bus slot.
> + *
> + * The following return configurations are possible:
> + *
> + * - @retval != NULL Matching bus with a free slot found.
> + *
> + * - @retval == NULL No matching bus with a free slot was found.
> + * For (@name != NULL) calls, a further distinction is
> + * made for this case:
> + *
> + * - address@hidden == NULL Bus not found by requested name. For
> recursive calls,
> + * the search should continue with siblings of @bus.
> + *
> + * - address@hidden != NULL Bus found by name, but it has no free slot.
> For
> + * recursive calls, this is an abort condition; names are
> + * unique.
> + *
> + * Parameters:
> + *
> + * - @bus root of subtree to search
> + *
> + * - @name if non-NULL, restrict search by this bus name
> + *
> + * - @type if non-NULL, restrict search by this bus type
> + *
> + * - @errp Returns the error if bus is found by name but it has no free
> slot.
> + * (@errp != NULL && address@hidden == NULL) must hold on input
> for all
> + * calls.
> + */
Thanks for going the extra mile and documenting the function's
non-trivial contract.
> static BusState *qbus_find_recursive(BusState *bus, const char *name,
> const char *type, Error **errp)
> {
> @@ -304,8 +338,7 @@ static BusState *qbus_find_recursive(BusState *bus, const
> char *name,
>
> /* bus is full -- fatal error for search by name */
> if (name != NULL) {
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
> - bus->name);
> + error_setg(errp, "Bus '%s' is full", bus->name);
> return NULL;
> }
> }
> @@ -317,8 +350,8 @@ static BusState *qbus_find_recursive(BusState *bus, const
> char *name,
> QLIST_FOREACH(child, &dev->child_bus, sibling) {
> BusState *ret;
>
> - ret = qbus_find_recursive(child, name, type, NULL);
> - if (ret) {
> + ret = qbus_find_recursive(child, name, type, errp);
> + if (ret || error_is_set(errp)) {
> return ret;
> }
> }
> @@ -338,13 +371,20 @@ static BusState *qbus_find(const char *path)
> bus = sysbus_get_default();
> pos = 0;
> } else {
> + Error *err = NULL;
> +
> if (sscanf(path, "%127[^/]%n", elem, &len) != 1) {
> assert(!path[0]);
> elem[0] = len = 0;
> }
> - bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, NULL);
> + bus = qbus_find_recursive(sysbus_get_default(), elem, NULL, &err);
> if (!bus) {
> - qerror_report(QERR_BUS_NOT_FOUND, elem);
> + if (error_is_set(&err)) {
> + qerror_report_err(err);
> + error_free(err);
> + } else {
> + qerror_report(QERR_BUS_NOT_FOUND, elem);
> + }
> return NULL;
> }
> pos = len;
> @@ -460,8 +500,9 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> }
> } else {
> bus = qbus_find_recursive(sysbus_get_default(), NULL, k->bus_type,
> - NULL);
> + &local_err);
> if (!bus) {
> + assert(!error_is_set(&local_err));
> qerror_report(QERR_NO_BUS_FOR_DEVICE,
> k->bus_type, driver);
> return NULL;
Straightforward way to lift the error reporting.
I didn't like the function before, and now I like it even less, as
lifting the error reporting complicates its contract further.
Note how many words you need to explain how to interpret
qbus_find_recursive()'s results. I believe that's because it tries to
do two similar, but different things: find a bus of a certain type with
a free slot, or find look up a bus by name, which must have a free slot
(else error). Three outcomes:
* Not found (both)
* Found, but no free slot (by name only)
* Found, has free slot (both)
I'd try to split the pie differently. Have a function to walk the qbus
tree, taking parameters bool (*test)(BusState *, void *opaque), void
*opaque. For each qbus, call test(qbus, opaque). If it returns true,
stop the search and return qbus. Else continue walk. If walk ends
without test() returning true, return NULL. No Error * anywhere.
Lookup by name should be trivial using this walker. If the walker
returns NULL, error bus not found. Else, if the returned bus has no
free slots, error bus full. Else it's good.
Searching for a bus of a certain type with space should be just as easy.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 10/15] qbus_find_recursive(): push Error and make it terminate a recursive search,
Markus Armbruster <=