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

[Ruifeng Bian]

----- Original Message -----
> From: "Michal Privoznik" <mprivozn at redhat.com>
> To: "Ruifeng Bian" <rbian at redhat.com>, libvir-list at 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?

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
>