Re: [Qemu-devel] [PATCH v1 4/4] s390x/zpci: properly fail if the zPCI device cannot be created

[David Hildenbrand <david@redhat.com>]

On 05.11.18 13:41, Cornelia Huck wrote:
> On Mon, 5 Nov 2018 13:04:04 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2018-11-05 12:03, David Hildenbrand wrote:
>>> Right now, errors during realize()/pre_plug/plug of the zPCI device
>>> would result in QEMU crashing instead of failing nicely when creating
>>> a zPCI device for a PCI device.
> 
> Yeah, failing instead of crashing is better :)
> 
> Is there any way we can trigger this problem for testing?

I guess trying to add more PCI devices (with implicit zPCI devices
getting created) than we have zPCI slots should be enough. So making
e.g. s390_pci_alloc_idx() fail.

(FH_MASK_INDEX = 0x0000ffff implies 65536 devices, which is not really
easy to test ;) )

Same would happen when running out of uids ... more unlikely to happen ;)

> 
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  hw/s390x/s390-pci-bus.c | 25 +++++++++++++++++++------
>>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 1849f9d334..4939490c7c 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -778,17 +778,31 @@ static void s390_pci_msix_free(S390PCIBusDevice *pbdev)
>>>  }
>>>  
>>>  static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
>>> -                                             const char *target)
>>> +                                             const char *target, Error **errp)
>>>  {
>>> -    DeviceState *dev = NULL;
>>> +    Error *local_err = NULL;
>>> +    DeviceState *dev;
>>>  
>>>      dev = qdev_try_create(BUS(s->bus), TYPE_S390_PCI_DEVICE);
>>>      if (!dev) {
>>> +        error_setg(errp, "zPCI device could not be created");
>>>          return NULL;
>>>      }
>>>  
>>> -    qdev_prop_set_string(dev, "target", target);
>>> -    qdev_init_nofail(dev);
>>> +    object_property_set_str(OBJECT(dev), "target", target, &local_err);
>>> +    if (local_err) {
>>> +        object_unparent(OBJECT(dev));
>>> +        error_propagate_prepend(errp, local_err,
>>> +                                "zPCI device could not be created: ");
>>> +        return NULL;
>>> +    }
>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
>>> +    if (local_err) {
>>> +        object_unparent(OBJECT(dev));
>>> +        error_propagate_prepend(errp, local_err,
>>> +                                "zPCI device could not be created: ");
>>> +        return NULL;
>>> +    }
>>>  
>>>      return S390_PCI_DEVICE(dev);
>>>  }
>>> @@ -873,9 +887,8 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>  
>>>          pbdev = s390_pci_find_dev_by_target(s, dev->id);
>>>          if (!pbdev) {
>>> -            pbdev = s390_pci_device_new(s, dev->id);
>>> +            pbdev = s390_pci_device_new(s, dev->id, errp);
>>>              if (!pbdev) {
>>> -                error_setg(errp, "create zpci device failed");
>>>                  return;
>>>              }
>>>          }
>>>   
>>
>> Looks right to me, I think this is even suitable for v3.1.
> 
> I can consider this for 3.1. Is this patch standalone?

Yes I think so.

> 
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>>
> 


-- 

Thanks,

David / dhildenb