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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Philippe Mathieu-Daudé <f4bug@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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?