6:22 a.m.
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.
Coldplug a scsi disk device without checking the address type in current
version, this patch also fix the scsi disk problem.
---
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 91eb661..af926fc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
if (virDomainDefAddImplicitControllers(vmdef) < 0)
return -1;
+ /* scsi disk should have an address type of driver */
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
+ (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("scsi disk cannot have an address of type
'%s'"),
+ virDomainDeviceAddressTypeToString(disk->info.type));
+ return -1;
+ }
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
return -1;
break;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 63fafa6..4226650 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
priv->qemuCaps,
disk->dst))
goto cleanup;
+
+ /* virtio device should either have a ccw or pci address */
+ 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 (!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 cleaup;
+ }
+ }
}
for (i = 0; i < vm->def->ndisks; i++) {
--
2.4.3
7:07 a.m.
On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
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.
Coldplug a scsi disk device without checking the address type in current
version, this patch also fix the scsi disk problem.
---
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 91eb661..af926fc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
if (virDomainDefAddImplicitControllers(vmdef) < 0)
return -1;
+ /* scsi disk should have an address type of driver */
+ if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
+ (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("scsi disk cannot have an address of type
'%s'"),
+ virDomainDeviceAddressTypeToString(disk->info.type));
+ return -1;
+ }
This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the
check for DISK_BUS_VIRTIO.
Should we check other bus types as well?
I think the most important part of the bug is that we allow hotplugging
a device with broken configuration, then fail to unplug it because it is
broken. Here detach-device works with --config even with an incorrect
address type.
if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) <
0)
return -1;
break;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 63fafa6..4226650 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
priv->qemuCaps,
disk->dst))
goto cleanup;
+
+ /* virtio device should either have a ccw or pci address */
+ 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;
+ }
Is hotplugging a device with a S390 type address impossible on a s390-ccw
machine?
+ } 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"));
These are not the only two options. At this point, the address type can
also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE
(and a PCI address will be generated by virDomainPCIAddressEnsureAddr).
The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For
ancient QEMUs which did not support -device, we use the PCI address to
uniquely identify the device. In that case, detaching a virtio disk with
no address is not possible.
For all reasonable versions of QEMU, the device alias is used, so
detaching devices with other address types is possible.
But we explicitly require a PCI or CCW address, to make sure we will
unplug only the device requested by user. S390 seems to be missing here,
but that might be on purpose.
Allowing S390 addresses on hotplug without allowing them on hotunplug
won't fix the linked bug for S390 machines, but attaching a device with
no PCI address is a valid use case - libvirt will just generate one.
+ goto cleaup;
Please make sure 'make check' and 'make syntax-check' pass before
sending a patch to the list, as described on our hacking page:
http://libvirt.org/hacking.html
Jan
2:05 a.m.
----- Original Message -----
From: "Ján Tomko" <jtomko(a)redhat.com>
To: "Ruifeng Bian" <rbian(a)redhat.com>
Cc: libvir-list(a)redhat.com
Sent: Tuesday, September 8, 2015 8:07:21 PM
Subject: Re: [libvirt] [PATCHv2] qemu: Validate address type when attaching a disk
device.
On Mon, Sep 07, 2015 at 07:22:01PM +0800, Ruifeng Bian wrote:
> 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.
>
> Coldplug a scsi disk device without checking the address type in current
> version, this patch also fix the scsi disk problem.
> ---
> src/qemu/qemu_driver.c | 8 ++++++++
> src/qemu/qemu_hotplug.c | 18 ++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 91eb661..af926fc 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8050,6 +8050,14 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr
> qemuCaps,
> if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> if (virDomainDefAddImplicitControllers(vmdef) < 0)
> return -1;
> + /* scsi disk should have an address type of driver */
> + if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> + (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("scsi disk cannot have an address of type
> '%s'"),
> +
> virDomainDeviceAddressTypeToString(disk->info.type));
> + return -1;
> + }
This is the check for DISK_BUS_SCSI, qemuDomainAssignAddresses does the
check for DISK_BUS_VIRTIO.
Should we check other bus types as well?
Yes, other bus types still have problem, I will collect all types.
I think the most important part of the bug is that we allow hotplugging
a device with broken configuration, then fail to unplug it because it is
broken. Here detach-device works with --config even with an incorrect
address type.
Detach-device should also check address types, I will add it.
> if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> return -1;
> break;
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 63fafa6..4226650 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -335,6 +335,24 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
> if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info,
> priv->qemuCaps,
> disk->dst))
> goto cleanup;
> +
> + /* virtio device should either have a ccw or pci address */
> + 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;
> + }
Is hotplugging a device with a S390 type address impossible on a s390-ccw
machine?
> + } 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"));
These are not the only two options. At this point, the address type can
also be VIRTIO_S390 for non-CCW S390 machines, and ADDRESS_TYPE_NONE
(and a PCI address will be generated by virDomainPCIAddressEnsureAddr).
ADDRESS_TYPE_NONE won't come here, since disk->info.type is 0.
The checks seem to be copied from qemuDomainDetachVirtioDiskDevice. For
ancient QEMUs which did not support -device, we use the PCI address to
uniquely identify the device. In that case, detaching a virtio disk with
no address is not possible.
For all reasonable versions of QEMU, the device alias is used, so
detaching devices with other address types is possible.
But we explicitly require a PCI or CCW address, to make sure we will
unplug only the device requested by user. S390 seems to be missing here,
but that might be on purpose.
Allowing S390 addresses on hotplug without allowing them on hotunplug
won't fix the linked bug for S390 machines, but attaching a device with
no PCI address is a valid use case - libvirt will just generate one.
Yes, the check code from qemuDomainDetachVirtioDiskDevice. I thought the code should
work both for attach and detach.
If user provide virtio device address for attachment, the address type should be PCI,
CCW or S390. We can check machine type to see if CCW address should be used. But how to
check S390 address should be used? Could I use virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_VIRTIO_S390)? In all other cases, PCI address should be used. Right?
> + goto cleaup;
Please make sure 'make check' and 'make syntax-check' pass before
sending a patch to the list, as described on our hacking page:
http://libvirt.org/hacking.html
Jan
3362
days inactive
3364
days old
2 comments
2 participants
participants (2)
-
Ján Tomko -
Ruifeng Bian