[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicat
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error |
Date: |
Mon, 21 Sep 2020 11:38:57 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
On 9/21/20 10:19 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
>> Commit 9940b2cfbc0 introduced qdev_realize() and qbus_realize()
>> with the ability to return a boolean value if an error occured,
>> thus the caller does not need to check if the Error* pointer is
>> set.
>> Provide the same ability to the BusRealize type.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> include/hw/qdev-core.h | 14 +++++++++++++-
>> hw/hyperv/vmbus.c | 5 +++--
>> hw/nubus/nubus-bus.c | 5 +++--
>> hw/pci/pci.c | 12 +++++++++---
>> hw/xen/xen-bus.c | 5 +++--
>> 5 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 02ac1c50b7f..eecfe794a71 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -32,7 +32,19 @@ typedef enum DeviceCategory {
>> typedef void (*DeviceRealize)(DeviceState *dev, Error **errp);
>> typedef void (*DeviceUnrealize)(DeviceState *dev);
>> typedef void (*DeviceReset)(DeviceState *dev);
>> -typedef void (*BusRealize)(BusState *bus, Error **errp);
>> +/**
>> + * BusRealize: Realize @bus.
>> + * @bus: bus to realize
>> + * @errp: pointer to error object
>> + *
>> + * On success, return true.
>> + * On failure, store an error through @errp and return false.
>> + */
>> +typedef bool (*BusRealize)(BusState *bus, Error **errp);
>> +/**
>> + * BusUnrealize: Unrealize @bus.
>> + * @bus: bus to unrealize
>> + */
>> typedef void (*BusUnrealize)(BusState *bus);
>>
>> /**
>> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
>> index 6ef895bc352..8a0452b2464 100644
>> --- a/hw/hyperv/vmbus.c
>> +++ b/hw/hyperv/vmbus.c
>> @@ -2487,7 +2487,7 @@ static const TypeInfo vmbus_dev_type_info = {
>> .instance_init = vmbus_dev_instance_init,
>> };
>>
>> -static void vmbus_realize(BusState *bus, Error **errp)
>> +static bool vmbus_realize(BusState *bus, Error **errp)
>> {
>> int ret = 0;
>> Error *local_err = NULL;
>> @@ -2519,7 +2519,7 @@ static void vmbus_realize(BusState *bus, Error **errp)
>> goto clear_event_notifier;
>> }
>>
>> - return;
>> + return true;
>>
>> clear_event_notifier:
>> event_notifier_cleanup(&vmbus->notifier);
>> @@ -2528,6 +2528,7 @@ remove_msg_handler:
>> error_out:
>> qemu_mutex_destroy(&vmbus->rx_queue_lock);
>> error_propagate(errp, local_err);
>> + return false;
>> }
>>
>> static void vmbus_unrealize(BusState *bus)
>> diff --git a/hw/nubus/nubus-bus.c b/hw/nubus/nubus-bus.c
>> index 942a6d5342d..d20d9c0f72c 100644
>> --- a/hw/nubus/nubus-bus.c
>> +++ b/hw/nubus/nubus-bus.c
>> @@ -65,12 +65,13 @@ static const MemoryRegionOps nubus_super_slot_ops = {
>> },
>> };
>>
>> -static void nubus_realize(BusState *bus, Error **errp)
>> +static bool nubus_realize(BusState *bus, Error **errp)
>> {
>> if (!nubus_find()) {
>> error_setg(errp, "at most one %s device is permitted",
>> TYPE_NUBUS_BUS);
>> - return;
>> + return false;
>> }
>> + return true;
>> }
>>
>> static void nubus_init(Object *obj)
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index de0fae10ab9..f535ebac847 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -115,7 +115,7 @@ static void pcibus_machine_done(Notifier *notifier, void
>> *data)
>> }
>> }
>>
>> -static void pci_bus_realize(BusState *qbus, Error **errp)
>> +static bool pci_bus_realize(BusState *qbus, Error **errp)
>> {
>> PCIBus *bus = PCI_BUS(qbus);
>>
>> @@ -123,13 +123,17 @@ static void pci_bus_realize(BusState *qbus, Error
>> **errp)
>> qemu_add_machine_init_done_notifier(&bus->machine_done);
>>
>> vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_pcibus, bus);
>> +
>> + return true;
>> }
>>
>> -static void pcie_bus_realize(BusState *qbus, Error **errp)
>> +static bool pcie_bus_realize(BusState *qbus, Error **errp)
>> {
>> PCIBus *bus = PCI_BUS(qbus);
>>
>> - pci_bus_realize(qbus, errp);
>> + if (!pci_bus_realize(qbus, errp)) {
>> + return false;
>> + }
>
> We now update bus->flags only when pci_bus_realize() succeeds. Is this
> a bug fix?
Fortunate side effect :) I'll let the PCI maintainers
have a look at it.
>
>>
>> /*
>> * A PCI-E bus can support extended config space if it's the root
>> @@ -144,6 +148,8 @@ static void pcie_bus_realize(BusState *qbus, Error
>> **errp)
>> bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
>> }
>> }
>> +
>> + return true;
>> }
>>
>> static void pci_bus_unrealize(BusState *qbus)
>> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
>> index 9ce1c9540b9..d7ef5d05e37 100644
>> --- a/hw/xen/xen-bus.c
>> +++ b/hw/xen/xen-bus.c
>> @@ -444,7 +444,7 @@ static void xen_bus_unrealize(BusState *bus)
>> }
>> }
>>
>> -static void xen_bus_realize(BusState *bus, Error **errp)
>> +static bool xen_bus_realize(BusState *bus, Error **errp)
>> {
>> XenBus *xenbus = XEN_BUS(bus);
>> unsigned int domid;
>> @@ -478,10 +478,11 @@ static void xen_bus_realize(BusState *bus, Error
>> **errp)
>> "failed to set up enumeration watch: ");
>> }
>>
>> - return;
>> + return true;
>>
>> fail:
>> xen_bus_unrealize(bus);
>> + return false;
>> }
>>
>> static void xen_bus_unplug_request(HotplugHandler *hotplug,
>
> I can't see an actual use of the new return value. Am I blind?
You aren't, I'm trying to make a 240 patches series digestible
by splitting it. One device is a (hotplug) PCIe bridge, as we
can plug/unplug it, this calls multiple realize/unrealize, and
I want to be sure the children objects are properly realized
so I care about this return value.
As it seems an improvement from an API PoV (following your recent
cleanup and code style change: simplify returning boolean for Error
instead of checking *errp is set). I thought merging it the sooner
is better, but I don't have problem reposting that later.
Regards,
Phil.