Re: [libvirt] [PATCH 2/3] qemu: Separate calls based on controller bus type

On 10/08/2013 02:45 PM, Eric Farman wrote: > For systems without a PCI bus, attaching a SCSI controller fails: > > [root@xxxxxxxx ~]# cat controller.xml > <controller type='scsi' model='virtio-scsi' index='0' /> > [root@xxxxxxxx ~]# virsh attach-device guest01 controller.xml > error: Failed to attach device from controller.xml > error: XML error: No PCI buses available > > A similar problem occurs with the detach of a controller: > > [root@xxxxxxxx ~]# virsh detach-device guest01 controller.xml > error: Failed to detach device from controller.xml > error: operation failed: controller scsi:0 not found > > The qemuDomainXXtachPciControllerDevice routines made assumptions > that any caller had a PCI bus. These routines now selectively calls > PCI functions where necessary, and assigns the device information > type to one appropriate for the bus in use. > > Signed-off-by: Eric Farman <farman(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_hotplug.c | 45 +++++++++++++++++++++++++++++++-------------- > 1 file changed, 31 insertions(+), 14 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index bc0f57a..70a1b3d 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -365,9 +365,23 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, > return -1; > } > > + if (!controller->info.type) { > + if (STREQLEN(vm->def->os.machine, "s390-ccw", 8) && STRPREFIX(vm->def->os.machine, "s390-ccw") should do the same

> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) > + controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; > + else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) > + controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390; > + } > + > if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > - if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) > - goto cleanup; > + if (!controller->info.type || I'd prefer a comparison against VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE instead.

> + controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > + if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &controller->info) < 0) > + goto cleanup; > + } else if (controller->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { > + if (qemuDomainCCWAddressAssign(&controller->info, priv->ccwaddrs, true) < 0) > + goto cleanup; > + } > releaseaddr = true; > if (qemuAssignDeviceControllerAlias(controller) < 0) > goto cleanup; > @@ -399,7 +413,8 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver, > qemuDomainObjExitMonitor(driver, vm); > > if (ret == 0) { > - controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; > + if (!controller->info.type) Here too. > + controller->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; > virDomainControllerInsertPreAlloced(vm->def, controller); > } > > @@ -3024,18 +3039,20 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver, > > detach = vm->def->controllers[idx]; > > - if (!virDomainDeviceAddressIsValid(&detach->info, > - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { > - virReportError(VIR_ERR_OPERATION_FAILED, "%s", > - _("device cannot be detached without a PCI address")); > - goto cleanup; > - } > + if (detach->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > + if (!virDomainDeviceAddressIsValid(&detach->info, > + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("device cannot be detached without a PCI address")); > + goto cleanup; > + } Shouldn't we check if there is either a VIRTIO_S390, VIRTIO_CCW or a PCI address?

> > - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { > - virReportError(VIR_ERR_OPERATION_FAILED, > - _("cannot hot unplug multifunction PCI device: %s"), > - dev->data.disk->dst); > - goto cleanup; > + if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("cannot hot unplug multifunction PCI device: %s"), > + dev->data.disk->dst); > + goto cleanup; > + } > } > > if (qemuDomainControllerIsBusy(vm, detach)) { > Jan