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

[Michal Privoznik]

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