[libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests

Yi Min Zhao posted 13 patches 7 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
Posted by Yi Min Zhao 7 years, 5 months ago
This commit adds hotplug support for PCI devices on S390 guests.
There's no need to implement hot unplug for zPCI as QEMU implements
an unplug callback which will unplug both PCI and zPCI device in a
cascaded way.
Currently, the following PCI devices are supported:
  virtio-blk-pci
  virtio-net-pci
  virtio-rng-pci
  virtio-input-host-pci
  virtio-keyboard-pci
  virtio-mouse-pci
  virtio-tablet-pci
  vfio-pci
  SCSIVhost device

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_hotplug.c | 151 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 141 insertions(+), 10 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 4f290b5648..65d25d776b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -154,6 +154,70 @@ qemuHotplugPrepareDiskAccess(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuDomainAttachZPCIDevice(qemuMonitorPtr mon,
+                           virDomainDeviceInfoPtr info)
+{
+    int ret = -1;
+    char *devstr_zpci = NULL;
+
+    if (!(devstr_zpci = qemuBuildZPCIDevStr(info)))
+        goto cleanup;
+
+    if (qemuMonitorAddDevice(mon, devstr_zpci) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(devstr_zpci);
+    return ret;
+}
+
+
+static int
+qemuDomainDetachZPCIDevice(qemuMonitorPtr mon,
+                           virDomainDeviceInfoPtr info)
+{
+    char *zpciAlias = NULL;
+    int ret = -1;
+
+    if (virAsprintf(&zpciAlias, "zpci%d", info->addr.pci.zpci.uid) < 0)
+        goto cleanup;
+
+    if (qemuMonitorDelDevice(mon, zpciAlias) < 0)
+        goto cleanup;
+
+    ret = 0;
+
+ cleanup:
+    VIR_FREE(zpciAlias);
+    return ret;
+}
+
+
+static int
+qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
+                                virDomainDeviceInfoPtr info)
+{
+    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
+        return qemuDomainAttachZPCIDevice(mon, info);
+
+    return 0;
+}
+
+
+static int
+qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
+                                virDomainDeviceInfoPtr info)
+{
+    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
+        return qemuDomainDetachZPCIDevice(mon, info);
+
+    return 0;
+}
+
+
 static int
 qemuHotplugWaitForTrayEject(virDomainObjPtr vm,
                             virDomainDiskDefPtr disk)
@@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
     if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
         goto exit_monitor;
 
-    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
+        goto exit_monitor;
+
+    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));
         goto exit_monitor;
+    }
 
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         ret = -2;
@@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
         goto cleanup;
 
     qemuDomainObjEnterMonitor(driver, vm);
-    ret = qemuMonitorAddDevice(priv->mon, devstr);
+
+    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
+        ignore_value(qemuDomainObjExitMonitor(driver, vm));
+        goto cleanup;
+    }
+
+    if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0)
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &controller->info));
+
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         releaseaddr = false;
         ret = -1;
@@ -1377,7 +1454,8 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
             goto cleanup;
     }
 
-    if (qemuDomainIsS390CCW(vm->def) &&
+    if (net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+        qemuDomainIsS390CCW(vm->def) &&
         virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CCW)) {
         net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
         if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def)))
@@ -1447,7 +1525,15 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
         goto try_remove;
 
     qemuDomainObjEnterMonitor(driver, vm);
+
+    if (qemuDomainAttachExtensionDevice(priv->mon, &net->info) < 0) {
+        ignore_value(qemuDomainObjExitMonitor(driver, vm));
+        virDomainAuditNet(vm, NULL, net, "attach", false);
+        goto try_remove;
+    }
+
     if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &net->info));
         ignore_value(qemuDomainObjExitMonitor(driver, vm));
         virDomainAuditNet(vm, NULL, net, "attach", false);
         goto try_remove;
@@ -1665,8 +1751,15 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
         goto error;
 
     qemuDomainObjEnterMonitor(driver, vm);
-    ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
-                                     configfd, configfd_name);
+
+    if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0)
+        goto exit_monitor;
+
+    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
+                                          configfd, configfd_name)) < 0)
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
+
+ exit_monitor:
     if (qemuDomainObjExitMonitor(driver, vm) < 0)
         goto error;
 
@@ -2322,8 +2415,13 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
     if (qemuMonitorAddObject(priv->mon, &props, &objAlias) < 0)
         goto exit_monitor;
 
-    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+    if (qemuDomainAttachExtensionDevice(priv->mon, &rng->info) < 0)
+        goto exit_monitor;
+
+    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &rng->info));
         goto exit_monitor;
+    }
 
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         releaseaddr = false;
@@ -2816,8 +2914,16 @@ qemuDomainAttachSCSIVHostDevice(virQEMUDriverPtr driver,
 
     qemuDomainObjEnterMonitor(driver, vm);
 
-    ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, vhostfdName);
+    if ((ret = qemuDomainAttachExtensionDevice(priv->mon, hostdev->info)) < 0)
+        goto exit_monitor;
+
+    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd,
+                                          vhostfdName)) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
+        goto exit_monitor;
+    }
 
+ exit_monitor:
     if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)
         goto audit;
 
@@ -3062,9 +3168,14 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
 
     release_backing = true;
 
-    if (qemuMonitorAddDevice(priv->mon, shmstr) < 0)
+    if (qemuDomainAttachExtensionDevice(priv->mon, &shmem->info) < 0)
         goto exit_monitor;
 
+    if (qemuMonitorAddDevice(priv->mon, shmstr) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &shmem->info));
+        goto exit_monitor;
+    }
+
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         release_address = false;
         goto cleanup;
@@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
         goto cleanup;
 
     qemuDomainObjEnterMonitor(driver, vm);
-    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
+
+    if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
+        if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0)
+            goto exit_monitor;
+    }
+
+    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
+        if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO)
+            ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info));
         goto exit_monitor;
+    }
 
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         releaseaddr = false;
@@ -3315,9 +3435,15 @@ qemuDomainAttachVsockDevice(virQEMUDriverPtr driver,
         goto cleanup;
 
     qemuDomainObjEnterMonitor(driver, vm);
-    if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0)
+
+    if (qemuDomainAttachExtensionDevice(priv->mon, &vsock->info) < 0)
         goto exit_monitor;
 
+    if (qemuMonitorAddDeviceWithFd(priv->mon, devstr, vsockPriv->vhostfd, fdname) < 0) {
+        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &vsock->info));
+        goto exit_monitor;
+    }
+
     if (qemuDomainObjExitMonitor(driver, vm) < 0) {
         releaseaddr = false;
         goto cleanup;
@@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
         qemuDomainMarkDeviceForRemoval(vm, &detach->info);
 
     qemuDomainObjEnterMonitor(driver, vm);
+    if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
+        qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) {
+        ignore_value(qemuDomainObjExitMonitor(driver, vm));
+        goto cleanup;
+    }
     if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
         ignore_value(qemuDomainObjExitMonitor(driver, vm));
         goto cleanup;
-- 
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
Posted by Andrea Bolognani 7 years, 5 months ago
On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> +static int
> +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
> +                                virDomainDeviceInfoPtr info)
> +{
> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
> +        return qemuDomainAttachZPCIDevice(mon, info);
> +
> +    return 0;

Same comment as with qemuBuildExtensionCommandLine().

[...]
> +static int
> +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
> +                                virDomainDeviceInfoPtr info)
> +{
> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
> +        return qemuDomainDetachZPCIDevice(mon, info);
> +
> +    return 0;

Here, too.

[...]
> @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>      if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
>          goto exit_monitor;
>  
> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> +    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
> +        goto exit_monitor;
> +

Since the zpci device refers to the underlying device through the
target= option, does it make sense to attach the zpci device first
and the target device second? I would expect it to fail unless you
attach them the other way around...

[...]
> @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> -    ret = qemuMonitorAddDevice(priv->mon, devstr);
> +
> +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }

I'm not sure this is entirely safe. Perhaps you should introduce an
exit_monitor label that ensures failure to exit the monitor is also
taken into account, and jump to that one instead?

[...]
> +    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> +                                          configfd, configfd_name)) < 0)
> +        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));

Parentheses around here, please.

[...]
> @@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> +
> +    if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
> +        if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0)
> +            goto exit_monitor;
> +    }
> +
> +    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> +        if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO)
> +            ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info));
>          goto exit_monitor;
> +    }

Shouldn't you rather check for the address type here? IIUC an input
device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for
example, and you don't want to try adding a zpci device in those
cases.

[...]
> @@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>          qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> +    if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +        qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }
>      if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
>          ignore_value(qemuDomainObjExitMonitor(driver, vm));
>          goto cleanup;

This one too looks like it would benefit from an exit_monitor lable.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
Posted by Yi Min Zhao 7 years, 4 months ago

在 2018/9/11 下午11:21, Andrea Bolognani 写道:
> On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
> [...]
>> +static int
>> +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
>> +                                virDomainDeviceInfoPtr info)
>> +{
>> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
>> +        return qemuDomainAttachZPCIDevice(mon, info);
>> +
>> +    return 0;
> Same comment as with qemuBuildExtensionCommandLine().
yes.
>
> [...]
>> +static int
>> +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
>> +                                virDomainDeviceInfoPtr info)
>> +{
>> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
>> +        return qemuDomainDetachZPCIDevice(mon, info);
>> +
>> +    return 0;
> Here, too.
I posted my idea in previous mail. Let's see that.
>
> [...]
>> @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>>       if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
>>           goto exit_monitor;
>>   
>> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
>> +    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
>> +        goto exit_monitor;
>> +
> Since the zpci device refers to the underlying device through the
> target= option, does it make sense to attach the zpci device first
> and the target device second? I would expect it to fail unless you
> attach them the other way around...
This attaching order is right. Qemu requires that attach zpci first and 
target device second.
In Qemu, zpci is created firstly and ready to build connection with 
target device, and after
target device is plugged, there's a mapping built for zpci and target 
device. If the order is
reversed, there would be error.
>
> [...]
>> @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
>>           goto cleanup;
>>   
>>       qemuDomainObjEnterMonitor(driver, vm);
>> -    ret = qemuMonitorAddDevice(priv->mon, devstr);
>> +
>> +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
>> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +        goto cleanup;
>> +    }
> I'm not sure this is entirely safe. Perhaps you should introduce an
> exit_monitor label that ensures failure to exit the monitor is also
> taken into account, and jump to that one instead?
I see there're a lot of code ignoring monitor exit failure. Of course, 
taking it into account
is proper. Do you strongly suggest to add that?
>
> [...]
>> +    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
>> +                                          configfd, configfd_name)) < 0)
>> +        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
> Parentheses around here, please.
ok.
>
> [...]
>> @@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
>>           goto cleanup;
>>   
>>       qemuDomainObjEnterMonitor(driver, vm);
>> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
>> +
>> +    if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
>> +        if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0)
>> +            goto exit_monitor;
>> +    }
>> +
>> +    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
>> +        if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO)
>> +            ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info));
>>           goto exit_monitor;
>> +    }
> Shouldn't you rather check for the address type here? IIUC an input
> device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for
> example, and you don't want to try adding a zpci device in those
> cases.
Yes, you're right. As my idea regarding using switch in attach***() and 
detach***(),
we could move the check there.
>
> [...]
>> @@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>>           qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>>   
>>       qemuDomainObjEnterMonitor(driver, vm);
>> +    if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
>> +        qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) {
>> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +        goto cleanup;
>> +    }
>>       if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
>>           ignore_value(qemuDomainObjExitMonitor(driver, vm));
>>           goto cleanup;
> This one too looks like it would benefit from an exit_monitor lable.
>
Exactly. I will reorganise the code.

-- 
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
Posted by Andrea Bolognani 7 years, 4 months ago
On Mon, 2018-09-17 at 14:10 +0800, Yi Min Zhao wrote:
> 在 2018/9/11 下午11:21, Andrea Bolognani 写道:
> > > @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
> > >       if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
> > >           goto exit_monitor;
> > >   
> > > -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> > > +    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
> > > +        goto exit_monitor;
> > > +
> > 
> > Since the zpci device refers to the underlying device through the
> > target= option, does it make sense to attach the zpci device first
> > and the target device second? I would expect it to fail unless you
> > attach them the other way around...
> 
> This attaching order is right. Qemu requires that attach zpci first and 
> target device second.
> In Qemu, zpci is created firstly and ready to build connection with 
> target device, and after
> target device is plugged, there's a mapping built for zpci and target 
> device. If the order is
> reversed, there would be error.

Cool, let's leave it as it is then.

> > [...]
> > > @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
> > >           goto cleanup;
> > >   
> > >       qemuDomainObjEnterMonitor(driver, vm);
> > > -    ret = qemuMonitorAddDevice(priv->mon, devstr);
> > > +
> > > +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
> > > +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> > > +        goto cleanup;
> > > +    }
> > 
> > I'm not sure this is entirely safe. Perhaps you should introduce an
> > exit_monitor label that ensures failure to exit the monitor is also
> > taken into account, and jump to that one instead?
> 
> I see there're a lot of code ignoring monitor exit failure. Of course, 
> taking it into account
> is proper. Do you strongly suggest to add that?

As mentioned a while ago, I'm not particularly comfortable around
this part of libvirt, so we should definitely have someone with
more experience look it over before merging.

That said, while there are indeed a few existing places where the
return value of qemuDomainObjExitMonitor() is ignored, it seems
like it's far more common to take it into account, so I would say
do that unless you have a very good reason not to.

> > [...]
> > > +    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> > > +                                          configfd, configfd_name)) < 0)
> > > +        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
> > 
> > Parentheses around here, please.
> 
> ok.

(Of course I meant curly braces, not parentheses :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
Posted by Yi Min Zhao 7 years, 4 months ago

在 2018/9/17 下午8:40, Andrea Bolognani 写道:
>>>>    
>>>>        qemuDomainObjEnterMonitor(driver, vm);
>>>> -    ret = qemuMonitorAddDevice(priv->mon, devstr);
>>>> +
>>>> +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
>>>> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>>>> +        goto cleanup;
>>>> +    }
>>> I'm not sure this is entirely safe. Perhaps you should introduce an
>>> exit_monitor label that ensures failure to exit the monitor is also
>>> taken into account, and jump to that one instead?
>> I see there're a lot of code ignoring monitor exit failure. Of course,
>> taking it into account
>> is proper. Do you strongly suggest to add that?
> As mentioned a while ago, I'm not particularly comfortable around
> this part of libvirt, so we should definitely have someone with
> more experience look it over before merging.
>
> That said, while there are indeed a few existing places where the
> return value of qemuDomainObjExitMonitor() is ignored, it seems
> like it's far more common to take it into account, so I would say
> do that unless you have a very good reason not to.
>
I will take it into account in next version.

-- 
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list