* [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, Laurent Vivier, Philippe Mathieu-Daudé, xen-devel, Anthony Perard, Paolo Bonzini To ease error checking in DeviceRealize handlers, let BusRealize return a boolean value, as qbus_realize() does. Having DeviceRealize similarly return a boolean value is left as an exercice for volunteers :) Philippe Mathieu-Daudé (2): qdev: Document qbus_realize() and qbus_unrealize() qdev: Let BusRealize() return a boolean value to indicate error include/hw/qdev-core.h | 26 +++++++++++++++++++++++++- 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, 43 insertions(+), 10 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Laurent Vivier, Paolo Bonzini, Anthony Perard, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Marcel Apfelbaum, Michael S. Tsirkin, xen-devel, Philippe Mathieu-Daudé To ease error checking in DeviceRealize handlers, let BusRealize return a boolean value, as qbus_realize() does. Having DeviceRealize similarly return a boolean value is left as an exercice for volunteers :) Philippe Mathieu-Daudé (2): qdev: Document qbus_realize() and qbus_unrealize() qdev: Let BusRealize() return a boolean value to indicate error include/hw/qdev-core.h | 26 +++++++++++++++++++++++++- 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, 43 insertions(+), 10 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] qdev: Document qbus_realize() and qbus_unrealize() 2020-09-20 11:44 ` Philippe Mathieu-Daudé @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé -1 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, Laurent Vivier, Philippe Mathieu-Daudé, xen-devel, Anthony Perard, Paolo Bonzini Add some documentation for the qbus_realize() and qbus_unrealize() functions introduced in commit 9940b2cfbc0. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/qdev-core.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index e025ba9653f..02ac1c50b7f 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -675,7 +675,19 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); void qbus_create_inplace(void *bus, size_t size, const char *typename, DeviceState *parent, const char *name); BusState *qbus_create(const char *typename, DeviceState *parent, const char *name); +/** + * qbus_realize: Realize a bus + * @bus: bus to realize + * @errp: pointer to error object + * + * On success, return true. + * On failure, store an error through @errp and return false. + */ bool qbus_realize(BusState *bus, Error **errp); +/** + * qbus_realize: Unrealize a bus + * @bus: bus to unrealize + */ void qbus_unrealize(BusState *bus); /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/2] qdev: Document qbus_realize() and qbus_unrealize() @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Laurent Vivier, Paolo Bonzini, Anthony Perard, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Marcel Apfelbaum, Michael S. Tsirkin, xen-devel, Philippe Mathieu-Daudé Add some documentation for the qbus_realize() and qbus_unrealize() functions introduced in commit 9940b2cfbc0. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/qdev-core.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index e025ba9653f..02ac1c50b7f 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -675,7 +675,19 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque); void qbus_create_inplace(void *bus, size_t size, const char *typename, DeviceState *parent, const char *name); BusState *qbus_create(const char *typename, DeviceState *parent, const char *name); +/** + * qbus_realize: Realize a bus + * @bus: bus to realize + * @errp: pointer to error object + * + * On success, return true. + * On failure, store an error through @errp and return false. + */ bool qbus_realize(BusState *bus, Error **errp); +/** + * qbus_realize: Unrealize a bus + * @bus: bus to unrealize + */ void qbus_unrealize(BusState *bus); /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion, -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-20 11:44 ` Philippe Mathieu-Daudé @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé -1 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, Laurent Vivier, Philippe Mathieu-Daudé, xen-devel, Anthony Perard, Paolo Bonzini 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; + } /* * 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, -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-20 11:44 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-20 11:44 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: Laurent Vivier, Paolo Bonzini, Anthony Perard, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Marcel Apfelbaum, Michael S. Tsirkin, xen-devel, Philippe Mathieu-Daudé 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; + } /* * 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, -- 2.26.2 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-20 11:44 ` Philippe Mathieu-Daudé @ 2020-09-21 7:01 ` Paul Durrant -1 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2020-09-21 7:01 UTC (permalink / raw) To: 'Philippe Mathieu-Daudé', 'Markus Armbruster', qemu-devel Cc: 'Stefano Stabellini', 'Daniel P. Berrangé', 'Eduardo Habkost', 'Michael S. Tsirkin', 'Laurent Vivier', xen-devel, 'Anthony Perard', 'Paolo Bonzini' > -----Original Message----- > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of Philippe Mathieu-Daudé > Sent: 20 September 2020 12:44 > To: Markus Armbruster <armbru@redhat.com>; qemu-devel@nongnu.org > Cc: Laurent Vivier <laurent@vivier.eu>; Paolo Bonzini <pbonzini@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Daniel P. Berrangé > <berrange@redhat.com>; Eduardo Habkost <ehabkost@redhat.com>; Paul Durrant <paul@xen.org>; Marcel > Apfelbaum <marcel.apfelbaum@gmail.com>; Michael S. Tsirkin <mst@redhat.com>; xen- > devel@lists.xenproject.org; Philippe Mathieu-Daudé <f4bug@amsat.org> > Subject: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error > > 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 +++-- Acked-by: Paul Durrant <paul@xen.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-21 7:01 ` Paul Durrant 0 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2020-09-21 7:01 UTC (permalink / raw) To: 'Philippe Mathieu-Daudé', 'Markus Armbruster', qemu-devel Cc: 'Laurent Vivier', 'Paolo Bonzini', 'Anthony Perard', 'Stefano Stabellini', 'Daniel P. Berrangé', 'Eduardo Habkost', 'Marcel Apfelbaum', 'Michael S. Tsirkin', xen-devel > -----Original Message----- > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of Philippe Mathieu-Daudé > Sent: 20 September 2020 12:44 > To: Markus Armbruster <armbru@redhat.com>; qemu-devel@nongnu.org > Cc: Laurent Vivier <laurent@vivier.eu>; Paolo Bonzini <pbonzini@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Daniel P. Berrangé > <berrange@redhat.com>; Eduardo Habkost <ehabkost@redhat.com>; Paul Durrant <paul@xen.org>; Marcel > Apfelbaum <marcel.apfelbaum@gmail.com>; Michael S. Tsirkin <mst@redhat.com>; xen- > devel@lists.xenproject.org; Philippe Mathieu-Daudé <f4bug@amsat.org> > Subject: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error > > 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 +++-- Acked-by: Paul Durrant <paul@xen.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-20 11:44 ` Philippe Mathieu-Daudé @ 2020-09-21 8:19 ` Markus Armbruster -1 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2020-09-21 8:19 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, qemu-devel, Laurent Vivier, Paolo Bonzini, Anthony Perard, xen-devel 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? > > /* > * 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-21 8:19 ` Markus Armbruster 0 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2020-09-21 8:19 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: qemu-devel, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, Laurent Vivier, xen-devel, Anthony Perard, Paolo Bonzini 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? > > /* > * 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-21 8:19 ` Markus Armbruster @ 2020-09-21 9:38 ` Philippe Mathieu-Daudé -1 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-21 9:38 UTC (permalink / raw) To: Markus Armbruster, Michael S. Tsirkin Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, qemu-devel, Laurent Vivier, xen-devel, Anthony Perard, Paolo Bonzini 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-21 9:38 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-09-21 9:38 UTC (permalink / raw) To: Markus Armbruster, Michael S. Tsirkin Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, qemu-devel, Laurent Vivier, Paolo Bonzini, Anthony Perard, xen-devel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-21 9:38 ` Philippe Mathieu-Daudé @ 2020-09-21 12:58 ` Markus Armbruster -1 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2020-09-21 12:58 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, qemu-devel, Laurent Vivier, Paolo Bonzini, Anthony Perard, xen-devel Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > 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. If it's an observable change, the commit message must mention it. I'd put it in its own commit then. Even if it's not observable, explaining why in the commit message would help, I think. >> >>> >>> /* >>> * 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. I wonder why you need to realize buses. Current code only ever does that via device_set_realized() -> qbus_realize() -> object_property_set_bool() -> bus_set_realized(), i.e. when realizing the device that provides the bus. Even if you do need to, why can't you call qbus_realize()? It already returns true on success, false on failure. > 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. Reviewing API improvements is always hard when we can't see the users, yet. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-21 12:58 ` Markus Armbruster 0 siblings, 0 replies; 16+ messages in thread From: Markus Armbruster @ 2020-09-21 12:58 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S. Tsirkin, Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, qemu-devel, Laurent Vivier, xen-devel, Anthony Perard, Paolo Bonzini Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > 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. If it's an observable change, the commit message must mention it. I'd put it in its own commit then. Even if it's not observable, explaining why in the commit message would help, I think. >> >>> >>> /* >>> * 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. I wonder why you need to realize buses. Current code only ever does that via device_set_realized() -> qbus_realize() -> object_property_set_bool() -> bus_set_realized(), i.e. when realizing the device that provides the bus. Even if you do need to, why can't you call qbus_realize()? It already returns true on success, false on failure. > 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. Reviewing API improvements is always hard when we can't see the users, yet. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error 2020-09-20 11:44 ` Philippe Mathieu-Daudé @ 2020-09-20 22:56 ` Richard Henderson -1 siblings, 0 replies; 16+ messages in thread From: Richard Henderson @ 2020-09-20 22:56 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Markus Armbruster, qemu-devel Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Michael S. Tsirkin, Paul Durrant, Laurent Vivier, Paolo Bonzini, Anthony Perard, xen-devel On 9/20/20 4:44 AM, Philippe Mathieu-Daudé wrote: > Philippe Mathieu-Daudé (2): > qdev: Document qbus_realize() and qbus_unrealize() > qdev: Let BusRealize() return a boolean value to indicate error Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error @ 2020-09-20 22:56 ` Richard Henderson 0 siblings, 0 replies; 16+ messages in thread From: Richard Henderson @ 2020-09-20 22:56 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Markus Armbruster, qemu-devel Cc: Stefano Stabellini, Daniel P. Berrangé, Eduardo Habkost, Paul Durrant, Michael S. Tsirkin, Laurent Vivier, xen-devel, Anthony Perard, Paolo Bonzini On 9/20/20 4:44 AM, Philippe Mathieu-Daudé wrote: > Philippe Mathieu-Daudé (2): > qdev: Document qbus_realize() and qbus_unrealize() > qdev: Let BusRealize() return a boolean value to indicate error Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-09-21 13:03 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-20 11:44 [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error Philippe Mathieu-Daudé 2020-09-20 11:44 ` Philippe Mathieu-Daudé 2020-09-20 11:44 ` [PATCH 1/2] qdev: Document qbus_realize() and qbus_unrealize() Philippe Mathieu-Daudé 2020-09-20 11:44 ` Philippe Mathieu-Daudé 2020-09-20 11:44 ` [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error Philippe Mathieu-Daudé 2020-09-20 11:44 ` Philippe Mathieu-Daudé 2020-09-21 7:01 ` Paul Durrant 2020-09-21 7:01 ` Paul Durrant 2020-09-21 8:19 ` Markus Armbruster 2020-09-21 8:19 ` Markus Armbruster 2020-09-21 9:38 ` Philippe Mathieu-Daudé 2020-09-21 9:38 ` Philippe Mathieu-Daudé 2020-09-21 12:58 ` Markus Armbruster 2020-09-21 12:58 ` Markus Armbruster 2020-09-20 22:56 ` [PATCH 0/2] " Richard Henderson 2020-09-20 22:56 ` Richard Henderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.