----- Original Message ----- > From: "Michal Privoznik" <mprivozn@xxxxxxxxxx> > To: "Ruifeng Bian" <rbian@xxxxxxxxxx>, libvir-list@xxxxxxxxxx > Sent: Wednesday, September 16, 2015 11:32:27 PM > Subject: Re: [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? I have thought about it. I'm not sure whether other hypervisor will use it, so I put it here. Okay, moving this part to domain_conf is better, I will do it. Thanks, Ruifeng > > > > > - > > /* 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list