Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device.

From: "Michal Privoznik" <mprivozn(a)redhat.com&gt; To: "Ruifeng Bian" <rbian(a)redhat.com&gt;, libvir-list(a)redhat.com Sent: Wednesday, September 16, 2015 11:32:27 PM Subject: Re: [libvirt] [PATCH v3] qemu: Validate address type when attaching a disk device. On 09.09.2015 13:17, Ruifeng Bian wrote: > Bug fix for: 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. > > Attaching a disk device with --config option need to check address type. > Otherwise, the domain cloudn't be started normally. This patch add a basic > check for address type. > > Detaching a disk device with --config also need to check disk options, > add qemuCheckDiskConfig() method in qemuDomainDetachDeviceConfig(). > --- > src/qemu/qemu_command.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > src/qemu/qemu_driver.c | 2 ++ > src/qemu/qemu_hotplug.c | 22 ++++++++++++++++++++++ > 3 files changed, 67 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index ec5e3d4..a2c7483 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3579,12 +3579,54 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) > goto error; > } > } > + > + if (disk->info.type) { > + switch (disk->bus) { > + case VIR_DOMAIN_DISK_BUS_IDE: > + case VIR_DOMAIN_DISK_BUS_SCSI: > + case VIR_DOMAIN_DISK_BUS_SATA: > + case VIR_DOMAIN_DISK_BUS_FDC: > + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk cannot have an address of type > '%s'"), > + > virDomainDeviceAddressTypeToString(disk->info.type)); > + goto error; > + } > + break; > + case VIR_DOMAIN_DISK_BUS_USB: > + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("usb disk cannot have an address of type > '%s'"), > + > virDomainDeviceAddressTypeToString(disk->info.type)); > + goto error; > + } > + break; > + case VIR_DOMAIN_DISK_BUS_VIRTIO: > + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && > + disk->info.type != > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && > + disk->info.type != > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && > + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("virtio disk cannot have an address of > type '%s'"), > + > virDomainDeviceAddressTypeToString(disk->info.type)); > + goto error; > + } > + break; > + case VIR_DOMAIN_DISK_BUS_XEN: > + case VIR_DOMAIN_DISK_BUS_UML: > + case VIR_DOMAIN_DISK_BUS_SD: > + /* no address type currently, return false if address provided > */ > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk cannot have an address of type '%s'"), > + > virDomainDeviceAddressTypeToString(disk->info.type)); > + goto error; > + } > + } > return 0; > error: > return -1; > } While this is technically correct I wonder if this should live here or in the config parsing part. I mean, these constraints are not qemu specific, are they?

> > - > /* Check whether the device address is using either 'ccw' or default s390 > * address format and whether that's "legal" for the current qemu and/or > * guest os.machine type. This is the corollary to the code which doesn't > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 91eb661..61424b0 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -8205,6 +8205,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, > switch ((virDomainDeviceType) dev->type) { > case VIR_DOMAIN_DEVICE_DISK: > disk = dev->data.disk; > + if (qemuCheckDiskConfig(disk) < 0) > + return -1; > if (!(det_disk = virDomainDiskRemoveByName(vmdef, disk->dst))) { > virReportError(VIR_ERR_INVALID_ARG, > _("no target device %s"), disk->dst); > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 63fafa6..1f46647 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -335,6 +335,28 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, > if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, > priv->qemuCaps, > disk->dst)) > goto cleanup; > + > + 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; > + } > + } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_S390)) > { > + if (!virDomainDeviceAddressIsValid(&disk->info, > + > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)) > { > + virReportError(VIR_ERR_OPERATION_FAILED, "%s", > + _("device cannot be attached without a > valid S390 address")); > + goto cleanup; > + } > + } 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")); > + goto cleanup; > + } > } > > for (i = 0; i < vm->def->ndisks; i++) { > Otherwise this is looking good. Michal