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