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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
