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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Philippe Mathieu-Daudé <f4bug@xxxxxxxxx> writes:

> On 9/21/20 10:19 AM, Markus Armbruster wrote:
>> 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?
>
> 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.