On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1257844 > > Attach-device can hotplug a virtio disk device with any address type now, > it need to validate the address type before the attachment. > > Coldplug a scsi disk device without checking the address type in current > version, this patch also fix the scsi disk problem. > --- > src/qemu/qemu_driver.c | 8 ++++++++ > src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 91eb661..af926fc 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps, > if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) > if (virDomainDefAddImplicitControllers(vmdef) < 0) > return -1; > + /* scsi disk should have an address type of driver */ > + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI && > + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("scsi disk cannot have an address of type '%s'"), > + virDomainDeviceAddressTypeToString(disk->info.type)); > + return -1; > + } This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the check for DISK_BUS_VIRTIO. Should we check other bus types as well? I think the most important part of the bug is that we allow hotplugging a device with broken configuration, then fail to unplug it because it is broken. Here detach-device works with --config even with an incorrect address type. > if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0) > return -1; > break; > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 63fafa6..4226650 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, > if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, > disk->dst)) > goto cleanup; > + > + /* virtio device should either have a ccw or pci address */ > + if (qemuDomainMachineIsS390CCW(vm->def) && > + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW)) { > + if (!virDomainDeviceAddressIsValid(&disk->info, > + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW)) { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("device cannot be attached without a valid CCW address")); > + goto cleanup; > + } Is hotplugging a device with a S390 type address impossible on a s390-ccw machine? > + } else { > + if (!virDomainDeviceAddressIsValid(&disk->info, > + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("device cannot be attached without a valid PCI address")); These are not the only two options. At this point, the address type can also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE (and a PCI address will be generated by virDomainPCIAddressEnsureAddr). The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For ancient QEMUs which did not support -device, we use the PCI address to uniquely identify the device. In that case, detaching a virtio disk with no address is not possible. For all reasonable versions of QEMU, the device alias is used, so detaching devices with other address types is possible. But we explicitly require a PCI or CCW address, to make sure we will unplug only the device requested by user. S390 seems to be missing here, but that might be on purpose. Allowing S390 addresses on hotplug without allowing them on hotunplug won't fix the linked bug for S390 machines, but attaching a device with no PCI address is a valid use case - libvirt will just generate one. > + goto cleaup; Please make sure 'make check' and 'make syntax-check' pass before sending a patch to the list, as described on our hacking page: http://libvirt.org/hacking.html Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list