[libvirt] [PATCH v4] qemu: Validate address type when attaching a disk device.
Ruifeng Bian
rbian at redhat.com
Thu Oct 8 03:40:04 UTC 2015
----- Original Message -----
> From: "John Ferlan" <jferlan at redhat.com>
> To: "Ruifeng Bian" <rbian at redhat.com>, libvir-list at redhat.com
> Sent: Wednesday, September 30, 2015 5:31:32 AM
> Subject: Re: [libvirt] [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 at redhat.com>
> > ---
> > 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++) {
> >
>
More information about the libvir-list
mailing list