[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbu
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive() |
Date: |
Fri, 19 Jun 2015 14:47:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
Did you mean avalanche?
Am 13.06.2015 um 13:18 schrieb Markus Armbruster:
> Reproducer:
>
> $ qemu-system-x86_64 -nodefaults -device virtio-rng-pci -device
> virtio-rng-pci -device virtio-rng-device,bus=virtio-bus
> qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus
> 'virtio-bus' is full
> qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus
> 'virtio-bus' is full
> qemu-system-x86_64: -device virtio-rng-device,bus=virtio-bus: Bus
> 'virtio-bus' not found
>
> qbus_find_recursive() reports the "is full" error itself, and leaves
> reporting "not found" to its caller. The result is confusion. Write
> it a function contract that permits leaving all error reporting to the
> caller, and implement it. Update callers to detect and report "is
> full".
>
> Screwed up when commit 1395af6 added the max_dev limit and the "is
> full" error condition to enforce it.
>
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
> qdev-monitor.c | 62
> ++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 7dd62dd..1408c86 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -364,43 +364,55 @@ static DeviceState *qbus_find_dev(BusState *bus, char
> *elem)
> return NULL;
> }
>
> +static inline bool qbus_is_full(BusState *bus)
> +{
> + BusClass *bus_class = BUS_GET_CLASS(bus);
> + return bus_class->max_dev && bus->max_index >= bus_class->max_dev;
> +}
> +
> +/**
This should probably just be "/*".
I'll trust you on getting the functional logic right, can't do a full
review right now.
Andreas
> + * Search the tree rooted at @bus for a bus.
> + * If @name, search for a bus with that name. Note that bus names
> + * need not be unique. Yes, that's screwed up.
> + * Else search for a bus that is a subtype of @bus_typename.
> + * If more than one exists, prefer one that can take another device.
> + * Return the bus if found, else %NULL.
> + */
> static BusState *qbus_find_recursive(BusState *bus, const char *name,
> const char *bus_typename)
> {
> - BusClass *bus_class = BUS_GET_CLASS(bus);
> BusChild *kid;
> - BusState *child, *ret;
> - int match = 1;
> + BusState *pick, *child, *ret;
> + bool match;
>
> - if (name && (strcmp(bus->name, name) != 0)) {
> - match = 0;
> - } else if (bus_typename && !object_dynamic_cast(OBJECT(bus),
> bus_typename)) {
> - match = 0;
> - } else if ((bus_class->max_dev != 0) && (bus_class->max_dev <=
> bus->max_index)) {
> - if (name != NULL) {
> - /* bus was explicitly specified: return an error. */
> - qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
> - bus->name);
> - return NULL;
> - } else {
> - /* bus was not specified: try to find another one. */
> - match = 0;
> - }
> + assert(name || bus_typename);
> + if (name) {
> + match = !strcmp(bus->name, name);
> + } else {
> + match = !!object_dynamic_cast(OBJECT(bus), bus_typename);
> }
> - if (match) {
> - return bus;
> +
> + if (match && !qbus_is_full(bus)) {
> + return bus; /* root matches and isn't full */
> }
>
> + pick = match ? bus : NULL;
> +
> QTAILQ_FOREACH(kid, &bus->children, sibling) {
> DeviceState *dev = kid->child;
> QLIST_FOREACH(child, &dev->child_bus, sibling) {
> ret = qbus_find_recursive(child, name, bus_typename);
> - if (ret) {
> - return ret;
> + if (ret && !qbus_is_full(ret)) {
> + return ret; /* a descendant matches and isn't full */
> + }
> + if (ret && !pick) {
> + pick = ret;
> }
> }
> }
> - return NULL;
> +
> + /* root or a descendant matches, but is full */
> + return pick;
> }
>
> static BusState *qbus_find(const char *path)
> @@ -423,6 +435,10 @@ static BusState *qbus_find(const char *path)
> if (!bus) {
> qerror_report(QERR_BUS_NOT_FOUND, elem);
> return NULL;
> + } else if (qbus_is_full(bus)) {
> + qerror_report(ERROR_CLASS_GENERIC_ERROR, "Bus '%s' is full",
> + elem);
> + return NULL;
> }
> pos = len;
> }
> @@ -529,7 +545,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
> }
> } else if (dc->bus_type != NULL) {
> bus = qbus_find_recursive(sysbus_get_default(), NULL, dc->bus_type);
> - if (!bus) {
> + if (!bus || qbus_is_full(bus)) {
> qerror_report(ERROR_CLASS_GENERIC_ERROR,
> "No '%s' bus found for device '%s'",
> dc->bus_type, driver);
>
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
- [Qemu-devel] [PATCH v2 4/7] qdev-monitor: Fix check for full bus, (continued)
- [Qemu-devel] [PATCH v2 4/7] qdev-monitor: Fix check for full bus, Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH v2 6/7] qdev-monitor: Propagate errors through set_property(), Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH v2 1/7] qdev: Deprecated qdev_init() is finally unused, drop, Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH v2 2/7] qdev: Un-deprecate qdev_init_nofail(), Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive(), Markus Armbruster, 2015/06/13
- Re: [Qemu-devel] [PATCH v2 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive(),
Andreas Färber <=
- [Qemu-devel] [PATCH v2 5/7] qdev-monitor: Convert qbus_find() to Error, Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH v2 7/7] qdev-monitor: Propagate errors through qdev_device_add(), Markus Armbruster, 2015/06/13