----- Original Message ----- > From: "John Ferlan" <jferlan@xxxxxxxxxx> > To: "Ruifeng Bian" <rbian@xxxxxxxxxx>, libvir-list@xxxxxxxxxx > Sent: Wednesday, September 30, 2015 5:31:32 AM > Subject: Re: [PATCH v4] qemu: Validate address type when attaching a disk device. > > > > On 09/17/2015 03:35 AM, 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(). > > > > Add virDomainDeviceAddressTypeIsValid method in domain_conf to check disk > > address type. Address type should match with disk driver, report error > > messages if any mismatch. > > > > Signed-off-by: Ruifeng Bian <rbian@xxxxxxxxxx> > > --- > > src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++++++ > > src/conf/domain_conf.h | 1 + > > src/libvirt_private.syms | 1 + > > src/qemu/qemu_command.c | 7 +++++++ > > src/qemu/qemu_driver.c | 2 ++ > > src/qemu/qemu_hotplug.c | 22 ++++++++++++++++++++++ > > 6 files changed, 65 insertions(+) > > > > Just so this isn't left "hanging"... Michal has generated an alternate > method to address the issue, see: > > http://www.redhat.com/archives/libvir-list/2015-September/msg00909.html > > I think there could be "synergies" between the two and left some > thoughts in the other patch which use some of this code. > > Some other comments below since they may be useful... > > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > > index 6df1618..f86760b 100644 > > --- a/src/conf/domain_conf.c > > +++ b/src/conf/domain_conf.c > > @@ -3200,6 +3200,38 @@ int > > virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, > > return 0; > > } > > > > +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk) > > should be: > > bool > virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk) > > > +{ > > + 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) > > + return 1; > > return false; > > > + break; > > + case VIR_DOMAIN_DISK_BUS_USB: > > + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) > > + return 1; > > return false; > > > + 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) > > + return 1; > > return false; > > > + 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 > > */ > > + return 1; > > return false; > > > + } > > + } > > + return 0; > > return true; > > > +} > > + > > virDomainDeviceInfoPtr > > virDomainDeviceGetInfo(virDomainDeviceDefPtr device) > > { > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > > index f043554..337fe51 100644 > > --- a/src/conf/domain_conf.h > > +++ b/src/conf/domain_conf.h > > @@ -2543,6 +2543,7 @@ virDomainDeviceDefPtr > > virDomainDeviceDefCopy(virDomainDeviceDefPtr src, > > virDomainXMLOptionPtr > > xmlopt); > > int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, > > int type); > > +int virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk); > > s/int/bool, probably unnecessary though > > > virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr > > device); > > int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, > > virDomainDeviceInfoPtr src); > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > index 5b3da83..6cd5b9e 100644 > > --- a/src/libvirt_private.syms > > +++ b/src/libvirt_private.syms > > @@ -229,6 +229,7 @@ virDomainDefPostParse; > > virDomainDefSetMemoryInitial; > > virDomainDeleteConfig; > > virDomainDeviceAddressIsValid; > > +virDomainDeviceAddressTypeIsValid; > > probably unnecessary > > > virDomainDeviceAddressTypeToString; > > virDomainDeviceDefCheckUnsupportedMemoryDevice; > > virDomainDeviceDefCopy; > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > > index 25f57f2..56ba08d 100644 > > --- a/src/qemu/qemu_command.c > > +++ b/src/qemu/qemu_command.c > > @@ -3606,6 +3606,13 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk) > > goto error; > > } > > } > > + > > + if (virDomainDeviceAddressTypeIsValid(disk)) { > > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > > + _("disk cannot have an address of type '%s'"), > > + > > virDomainDeviceAddressTypeToString(disk->info.type)); > > how about disk target '%s' using bus '%s' cannot have an address of type > '%s', where target would be disk->dst and the bus would be decoded via > virDomainDiskBusTypeToString(disk->bus), results in error message such as: > > error: unsupported configuration: disk target 'vde' using bus 'virtio' > cannot have an address of type 'drive' Thanks, the message is clear now. > > > + goto error; > > + } > > But I'm not convinced the check should be put it here. > > > return 0; > > error: > > return -1; > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > index fcf86b6..26c1502 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -8208,6 +8208,8 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef, > > switch ((virDomainDeviceType) dev->type) { > > case VIR_DOMAIN_DEVICE_DISK: > > disk = dev->data.disk; > > + if (qemuCheckDiskConfig(disk) < 0) > > + return -1; > > Why are we checking at Detach? Doesn't that already fail? Or is this > just to get the "new" error message? If we just want to detach a disk without attaching it before, it's necessary to check the configuration here. > > > 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 e84a78d..6156243 100644 > > --- a/src/qemu/qemu_hotplug.c > > +++ b/src/qemu/qemu_hotplug.c > > @@ -336,6 +336,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; > > + } > > The whole purpose of qemuCheckCCWS390AddressSupport is to avoid adding > more places where these type lines need to be added. Yes, thanks. > > John > > > + } 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++) { > > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list