[PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error

[PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error

[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

[PATCH 1/2] qdev: Document qbus_realize() and qbus_unrealize()

[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

[PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[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

Re: [PATCH 0/2] qdev: Let BusRealize() return a boolean value to indicate error

[Richard Henderson]

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~

RE: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Paul Durrant]

> -----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>

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Markus Armbruster]

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?

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Philippe Mathieu-Daudé]

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.

Re: [PATCH 2/2] qdev: Let BusRealize() return a boolean value to indicate error

[Markus Armbruster]

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.