Summary: two nits, one in the docs and one at the end of this email. [Osier, I'm CCing you because there is some food for thought for SCSI]. On 01/05/2012 05:17 AM, Laine Stump wrote:

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 18b7e22..dcdf91f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1001,8 +1001,16 @@ "block", "dir", or "network" and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed - to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". The + to the guest OS. Possible values for this attribute are + "floppy", "disk", "cdrom", and "lun", defaulting to + "disk". "lun" (<span class="since">since 0.9.10</span>) is only + valid when type is "block" and the target element's "bus" + attribute is "virtio", and behaves identically to "disk", + except that generic SCSI commands from the guest are accepted

What about "are forwarded to the disk"? This is also true in the SCSI case (for SCSI, "block" will emulate commands rather than fail them; but for "lun" the behavior is identical).

+ - also note that device='lun' will only be recognized for + actual raw devices, never for individual partitions or LVM + partitions (in those cases, the kernel will reject the generic + SCSI commands, making it identical to device='disk'). The

Perhaps add a note that QEMU may fail to start? Again, this is not the case for virtio but it will be for "scsi".

optional<code>snapshot</code> attribute indicates the default behavior of the disk during disk snapshots: "internal" requires a file format such as qcow2 that can store both the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a8f7f4..2f8bec5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -794,6 +794,7 @@ <value>floppy</value> <value>disk</value> <value>cdrom</value> +<value>lun</value> </choice> </attribute> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 21113c6..a06942b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -157,7 +157,8 @@ VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST, VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST, "disk", "cdrom", - "floppy") + "floppy", + "lun")

VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST, "ide", @@ -3093,7 +3094,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) def->readonly = 1;

- if (def->device == VIR_DOMAIN_DISK_DEVICE_DISK&& + if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK || + def->device == VIR_DOMAIN_DISK_DEVICE_LUN)&& !STRPREFIX((const char *)target, "hd")&& !STRPREFIX((const char *)target, "sd")&& !STRPREFIX((const char *)target, "vd")&&

I wonder if you should extend this check to VIR_DOMAIN_DISK_DEVICE_CDROM as well.

+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + /* make sure that both the bus and the qemu binary support + * type='lun' (SG_IO). + */ + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for bus='%s'"), + bus); + goto error; + } + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for type='%s'"), + virDomainDiskTypeToString(disk->type)); + goto error; + } + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is not supported by this QEMU")); + goto error; + } + } + switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: virBufferAddLit(&opt, "ide-drive"); @@ -2050,6 +2074,14 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, virBufferAsprintf(&opt, ",event_idx=%s", virDomainVirtioEventIdxTypeToString(disk->event_idx)); } + if (qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI)) { + /* if sg_io is true but the scsi option isn't supported, + * that means it's just always on in this version of qemu. + */ + virBufferAsprintf(&opt, ",scsi=%s", + (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) + ? "on" : "off"); + } if (qemuBuildDeviceAddressStr(&opt,&disk->info, qemuCaps)< 0) goto error; break; @@ -4241,6 +4273,7 @@ qemuBuildCommandLine(virConnectPtr conn, bootFloppy = 0; break; case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: bootindex = bootDisk; bootDisk = 0; break;

Suboptimal, because a type='lun' disk could well be a CD. :/ But short of supporting bootindex directly in the <disk> and <network> items, we cannot do much about it though.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 82bab67..3c55216 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4987,6 +4987,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false); break;

Side note: this fails if you're trying to hot-plug a SCSI CD drive. Probably this error condition in qemuDomainChangeEjectableMedia: for (i = 0 ; i < vm->def->ndisks ; i++) { if (vm->def->disks[i]->bus == disk->bus && STREQ(vm->def->disks[i]->dst, disk->dst)) { origdisk = vm->def->disks[i]; break; } } if (!origdisk) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("No device with bus '%s' and target '%s'"), virDomainDiskBusTypeToString(disk->bus), disk->dst); return -1; } should instead fall back to device hot-plug for VIR_DOMAIN_DISK_BUS_SCSI.

case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm, disk); @@ -5109,6 +5110,7 @@ qemuDomainDetachDeviceDiskLive(struct qemud_driver *driver,

switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) ret = qemuDomainDetachPciDiskDevice(driver, vm, dev); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) @@ -9607,7 +9609,8 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) * that succeed as well */ for (i = 0; i< vm->def->ndisks; i++) { - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&& + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK || + vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN)&& (!vm->def->disks[i]->driverType || STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) { qemuReportError(VIR_ERR_OPERATION_INVALID,

I think that we should fail for device='lun' and format != 'raw'. Paolo

On 01/05/2012 08:59 AM, Laine Stump wrote:

...

...

@@ -9607,7 +9609,8 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) * that succeed as well */ for (i = 0; i< vm->def->ndisks; i++) { - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&& + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK || + vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN)&& (!vm->def->disks[i]->driverType || STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) { qemuReportError(VIR_ERR_OPERATION_INVALID, I think that we should fail for device='lun' and format != 'raw'.

Truthfully, I didn't totally understand what that code was doing, but it seemed appropriate to at least duplicate the behavior of DEVICE_DISK :-) Looking at it some more, I *think* what is necessary, is to always return false if there is a disk that is DEVICE_LUN, since really the only thing that can be snapshotted is a qcow2 disk, and DEVICE_LUN disks by definition are never qcow2. So does it seem reasonable to change this to:

- if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&& - (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) { + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN) || + (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&& + STRNEQ_NULLABLE(vm->def->disks[i]->driverType, "qcow2"))) { qemuReportError(VIR_ERR_OPERATION_INVALID,

Yes, that would be reasonable. This function is only called when doing an internal snapshot, which requires that all disk devices from the guest perspective are in qcow2 format on the host. But I agree that a device='lun' will never be in qcow2 format - if you plan on using raw SG_IO, then you plan on using the disk as-is (raw) and not wrapping it through another layer of qemu emulation. In short, we are safe requiring that an internal snapshot cannot be done if any device='lun' are present. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 01/05/2012 11:10 AM, Paolo Bonzini wrote:

On 01/05/2012 04:59 PM, Laine Stump wrote:

...

On 01/05/2012 03:44 AM, Paolo Bonzini wrote:

...

Summary: two nits, one in the docs and one at the end of this email.

[Osier, I'm CCing you because there is some food for thought for SCSI].

On 01/05/2012 05:17 AM, Laine Stump wrote:

...

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 18b7e22..dcdf91f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1001,8 +1001,16 @@ "block", "dir", or "network" and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed - to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". The + to the guest OS. Possible values for this attribute are + "floppy", "disk", "cdrom", and "lun", defaulting to + "disk". "lun" (<span class="since">since 0.9.10</span>) is only + valid when type is "block" and the target element's "bus" + attribute is "virtio", and behaves identically to "disk", + except that generic SCSI commands from the guest are accepted What about "are forwarded to the disk"?

Okay, I'm adding that to the end of the sentence.

...

This is also true in the SCSI case (for SCSI, "block" will emulate commands rather than fail them; but for "lun" the behavior is identical).

We can add that to the docs when virtio-blk-scsi support is added.

Actually, there's already support for SCSI devices with device='disk' or device='cdrom', just not for the virtio implementation of SCSI. So it could be added now.

It sounds like you're saying that when device='disk' & type='block' and dev='[some physical SCSI device], sg_io commands will be emulated for that device. That doesn't make sense, so you must be saying something else.

In the past, generic SCSI commands issued from a guest to a virtio disk were always passed through to the underlying disk by qemu, and the kernel would also pass them on. As a result of CVE-2011-4127 (see: http://seclists.org/oss-sec/2011/q4/536), qemu now honors its scsi=on|off device option for virtio-blk-pci (which enables/disables passthrough of generic SCSI commands), and the kernel will only allow the commands for physical devices (not for partitions or logical volumes). The default behavior of qemu is still to allow sending generic SCSI commands to physical disks that are presented to a guest as virtio-blk-pci devices, but libvirt prefers to disable those commands in the standard virtio block devices, enabling it only when specifically requested (hopefully indicating that the requester understands what they're asking for). For this purpose, a new libvirt disk device type (device='lun') has been created. device='lun' is identical to the default device='disk', except that: 1) It is only allowed if bus='virtio', type='block', and the qemu version is "new enough" to support it ("new enough" == qemu 0.11 or better), otherwise the domain will fail to start and a CONFIG_UNSUPPORTED error will be logged). 2) The option "scsi=on" will be added to the -device arg to allow SG_IO commands (if device !='lun', "scsi=off" will be added to the -device arg so that SG_IO commands are specifically forbidden). Guests which continue to use disk device='disk' (the default) will no longer be able to use SG_IO commands on the disk; those that have their disk device changed to device='lun' will still be able to use SG_IO commands. *docs/formatdomain.html.in - document the new device attribute value. *docs/schemas/domaincommon.rng - allow it in the RNG *tests/* - update the args of several existing tests to add scsi=off, and add one new test that will test scsi=on. *src/conf/domain_conf.c - update domain XML parser and formatter *src/qemu/qemu_(command|driver|hotplug).c - treat VIR_DOMAIN_DISK_DEVICE_LUN *almost* identically to VIR_DOMAIN_DISK_DEVICE_DISK, except as indicated above. Note that no support for this new device value was added to any hypervisor drivers other than qemu, because it's unclear what it might mean (if anything) to those drivers. --- v3 changes: 1) Add note to docs that the SCSI commands are not only accepted from the guest, but are passed through to the physical device (I didn't add anything about the SCSI commands being emulated because I still don't understand that situation perfectly, and don't want to add misleading docs about existing behavior. 2) Change the logic of qemuDomainSnapshotIsAllowed() to always return false if a device='lun' disk is encountered, regardless of the format. In reality, lun disks are always 'raw', so it would return false anyway, but this makes it clearer. I'm pretty sure all the other comments in reviews weren't directly related to the problem being addressed in this patch. Here is a diff of what I squashed in between v2 and v3: xdiff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in xindex dcdf91f..42e23a1 100644 x--- a/docs/formatdomain.html.in x+++ b/docs/formatdomain.html.in x@@ -1007,6 +1007,7 @@ valid when type is "block" and the target element's "bus" attribute is "virtio", and behaves identically to "disk", except that generic SCSI commands from the guest are accepted + and passed through to the physical device - also note that device='lun' will only be recognized for actual raw devices, never for individual partitions or LVM partitions (in those cases, the kernel will reject the generic xdiff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c xindex 3c55216..7be045f 100644 x--- a/src/qemu/qemu_driver.c x+++ b/src/qemu/qemu_driver.c x@@ -9609,10 +9609,9 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) * that succeed as well */ for (i = 0; i < vm->def->ndisks; i++) { - if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK || - vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN) && - (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) { + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN) || + (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK && + STRNEQ_NULLABLE(vm->def->disks[i]->driverType, "qcow2"))) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("Disk '%s' does not support snapshotting"), vm->def->disks[i]->src); docs/formatdomain.html.in | 13 ++++- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 6 ++- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 35 ++++++++++++- src/qemu/qemu_driver.c | 8 ++- src/qemu/qemu_hotplug.c | 4 +- tests/qemuhelptest.c | 24 ++++++--- .../qemuxml2argv-boot-complex-bootindex.args | 4 +- .../qemuxml2argv-boot-complex.args | 4 +- .../qemuxml2argvdata/qemuxml2argv-boot-order.args | 2 +- .../qemuxml2argv-disk-ioeventfd.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-disk-order.args | 4 +- .../qemuxml2argv-encrypted-disk.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-event_idx.args | 2 +- .../qemuxml2argvdata/qemuxml2argv-virtio-lun.args | 11 ++++ tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml | 57 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 22 ++++++-- tests/qemuxml2xmltest.c | 1 + 19 files changed, 171 insertions(+), 32 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 18b7e22..42e23a1 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1001,8 +1001,17 @@ "block", "dir", or "network" and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed - to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". The + to the guest OS. Possible values for this attribute are + "floppy", "disk", "cdrom", and "lun", defaulting to + "disk". "lun" (<span class="since">since 0.9.10</span>) is only + valid when type is "block" and the target element's "bus" + attribute is "virtio", and behaves identically to "disk", + except that generic SCSI commands from the guest are accepted + and passed through to the physical device + - also note that device='lun' will only be recognized for + actual raw devices, never for individual partitions or LVM + partitions (in those cases, the kernel will reject the generic + SCSI commands, making it identical to device='disk'). The optional <code>snapshot</code> attribute indicates the default behavior of the disk during disk snapshots: "internal" requires a file format such as qcow2 that can store both the diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 7a8f7f4..2f8bec5 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -794,6 +794,7 @@ <value>floppy</value> <value>disk</value> <value>cdrom</value> + <value>lun</value> </choice> </attribute> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 21113c6..a06942b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -157,7 +157,8 @@ VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST, VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST, "disk", "cdrom", - "floppy") + "floppy", + "lun") VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST, "ide", @@ -3093,7 +3094,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) def->readonly = 1; - if (def->device == VIR_DOMAIN_DISK_DEVICE_DISK && + if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK || + def->device == VIR_DOMAIN_DISK_DEVICE_LUN) && !STRPREFIX((const char *)target, "hd") && !STRPREFIX((const char *)target, "sd") && !STRPREFIX((const char *)target, "vd") && diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index cd882bb..ec1f013 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -230,6 +230,7 @@ enum virDomainDiskDevice { VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_DEVICE_FLOPPY, + VIR_DOMAIN_DISK_DEVICE_LUN, VIR_DOMAIN_DISK_DEVICE_LAST }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ea1b763..21d8877 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1876,7 +1876,8 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } if (bootable && qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_BOOT) && - disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && + (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK || + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) && disk->bus != VIR_DOMAIN_DISK_BUS_IDE) virBufferAddLit(&opt, ",boot=on"); if (disk->readonly && @@ -2022,6 +2023,29 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, goto error; } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { + /* make sure that both the bus and the qemu binary support + * type='lun' (SG_IO). + */ + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for bus='%s'"), + bus); + goto error; + } + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk device='lun' is not supported for type='%s'"), + virDomainDiskTypeToString(disk->type)); + goto error; + } + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk device='lun' is not supported by this QEMU")); + goto error; + } + } + switch (disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: virBufferAddLit(&opt, "ide-drive"); @@ -2050,6 +2074,14 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, virBufferAsprintf(&opt, ",event_idx=%s", virDomainVirtioEventIdxTypeToString(disk->event_idx)); } + if (qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI)) { + /* if sg_io is true but the scsi option isn't supported, + * that means it's just always on in this version of qemu. + */ + virBufferAsprintf(&opt, ",scsi=%s", + (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) + ? "on" : "off"); + } if (qemuBuildDeviceAddressStr(&opt, &disk->info, qemuCaps) < 0) goto error; break; @@ -4241,6 +4273,7 @@ qemuBuildCommandLine(virConnectPtr conn, bootFloppy = 0; break; case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: bootindex = bootDisk; bootDisk = 0; break; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 82bab67..7be045f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4987,6 +4987,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false); break; case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm, disk); @@ -5109,6 +5110,7 @@ qemuDomainDetachDeviceDiskLive(struct qemud_driver *driver, switch (disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) ret = qemuDomainDetachPciDiskDevice(driver, vm, dev); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) @@ -9607,9 +9609,9 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) * that succeed as well */ for (i = 0; i < vm->def->ndisks; i++) { - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK && - (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) { + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN) || + (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK && + STRNEQ_NULLABLE(vm->def->disks[i]->driverType, "qcow2"))) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("Disk '%s' does not support snapshotting"), vm->def->disks[i]->src); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f3597a1..c1ccfa9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -162,8 +162,10 @@ qemuDomainCheckEjectableMedia(struct qemud_driver *driver, virDomainDiskDefPtr disk = vm->def->disks[i]; struct qemuDomainDiskInfo info; - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK || + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) { continue; + } memset(&info, 0, sizeof(info)); diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 60155e7..1ef0d9b 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -329,7 +329,8 @@ mymain(void) QEMU_CAPS_DRIVE_AIO, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-kvm-0.12.1.2-rhel60", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -376,7 +377,8 @@ mymain(void) QEMU_CAPS_USB_HUB, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-kvm-0.12.3", 12003, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -416,7 +418,8 @@ mymain(void) QEMU_CAPS_DRIVE_AIO, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-kvm-0.13.0", 13000, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -472,7 +475,8 @@ mymain(void) QEMU_CAPS_USB_HUB, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-kvm-0.12.1.2-rhel61", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -524,7 +528,9 @@ mymain(void) QEMU_CAPS_USB_HUB, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SCSI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-kvm-0.12.1.2-rhel62-beta", 12001, 1, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -584,7 +590,9 @@ mymain(void) QEMU_CAPS_USB_HUB, QEMU_CAPS_NO_SHUTDOWN, QEMU_CAPS_PCI_ROMBAR, - QEMU_CAPS_NO_ACPI); + QEMU_CAPS_NO_ACPI, + QEMU_CAPS_VIRTIO_BLK_SCSI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("qemu-1.0", 1000000, 0, 0, QEMU_CAPS_VNC_COLON, QEMU_CAPS_NO_REBOOT, @@ -648,7 +656,9 @@ mymain(void) QEMU_CAPS_PCI_ROMBAR, QEMU_CAPS_ICH9_AHCI, QEMU_CAPS_NO_ACPI, - QEMU_CAPS_FSDEV_READONLY); + QEMU_CAPS_FSDEV_READONLY, + QEMU_CAPS_VIRTIO_BLK_SCSI, + QEMU_CAPS_VIRTIO_BLK_SG_IO); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-complex-bootindex.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-complex-bootindex.args index ae0b2b1..ed00c64 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-boot-complex-bootindex.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-complex-bootindex.args @@ -9,9 +9,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ -monitor unix:/tmp/test-monitor,server,nowait \ -no-acpi \ -drive file=/tmp/vda.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=3 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=3 \ -drive file=/tmp/vdb.img,if=none,id=drive-virtio-disk1 \ --device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk1,id=virtio-disk1 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk1,id=virtio-disk1 \ -drive file=/dev/HostVG/hda,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/hdb,if=none,id=drive-ide0-0-1 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-complex.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-complex.args index c8d32ec..cffb8ad 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-boot-complex.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-complex.args @@ -10,9 +10,9 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ -no-acpi \ -boot dnca \ -drive file=/tmp/vda.img,if=none,id=drive-virtio-disk0,boot=on \ --device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0 \ -drive file=/tmp/vdb.img,if=none,id=drive-virtio-disk1 \ --device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk1,id=virtio-disk1 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk1,id=virtio-disk1 \ -drive file=/dev/HostVG/hda,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ -drive file=/dev/HostVG/hdb,if=none,id=drive-ide0-0-1 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-order.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-order.args index 14367b1..9220987 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-boot-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-order.args @@ -12,7 +12,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \ -drive file=/root/boot.iso,if=none,media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,bootindex=1 \ -drive file=sheepdog:example.org:6000:image,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=3 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=3 \ -drive file=/dev/null,if=none,id=drive-fdc0-0-1 \ -global isa-fdc.driveB=drive-fdc0-0-1 \ -global isa-fdc.bootindexB=4 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-ioeventfd.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-ioeventfd.args index c512f15..2f4e7fd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-ioeventfd.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-ioeventfd.args @@ -3,7 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi \ -boot dc -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 \ -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,ioeventfd=on,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-pci,ioeventfd=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ -drive file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -device virtio-net-pci,tx=bh,ioeventfd=off,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-order.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-order.args index 6483e5e..0643072 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-order.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-order.args @@ -13,8 +13,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \ -drive file=/dev/HostVG/QEMUGuest2,if=none,media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -drive file=/tmp/data.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \ -drive file=/tmp/logs.img,if=none,id=drive-virtio-disk1 \ --device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,id=virtio-disk1 \ +-device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,id=virtio-disk1 \ -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk.args b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk.args index c6634fd..022f6ad 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-encrypted-disk.args @@ -5,6 +5,6 @@ encryptdisk -uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 -nographic -nodefconfig \ path=//var/lib/libvirt/qemu/encryptdisk.monitor,server,nowait -mon \ chardev=monitor,mode=readline -rtc base=utc -no-acpi -boot c -drive \ file=/storage/guest_disks/encryptdisk,if=none,id=drive-virtio-disk0,boot=on,\ -format=qcow2 -device virtio-blk-pci,bus=pci.0,addr=0x4,\ +format=qcow2 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x4,\ drive=drive-virtio-disk0,id=virtio-disk0 -usb -device virtio-balloon-pci,\ id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.args b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.args index f6ebb60..a506274 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-event_idx.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-event_idx.args @@ -3,7 +3,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi \ -boot dc -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 \ -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,event_idx=on,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ +-device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ -drive file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0 \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -device virtio-net-pci,event_idx=off,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args new file mode 100644 index 0000000..5df9182 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.args @@ -0,0 +1,11 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc-0.13 -m 1024 -smp 1 -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi \ +-boot dc -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 \ +-drive file=/dev/sdfake,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,scsi=on,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso,if=none,media=cdrom,id=drive-ide0-1-0 \ +-device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3 \ +-net user,vlan=0,name=hostnet0 -serial pty -usb -vnc 127.0.0.1:-809 -std-vga \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml new file mode 100644 index 0000000..abe1b2f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun.xml @@ -0,0 +1,57 @@ +<domain type='qemu'> + <name>test</name> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid> + <memory>1048576</memory> + <currentMemory>1048576</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.13'>hvm</type> + <boot dev='cdrom'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='lun'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/sdfake'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='qemu' type='raw'/> + <source file='/var/lib/libvirt/Fedora-14-x86_64-Live-KDE.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='ide' index='0'/> + <interface type='user'> + <mac address='52:54:00:e5:48:58'/> + <model type='virtio'/> + <driver name='vhost' event_idx='off'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5091' autoport='no' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + </graphics> + <video> + <model type='vga' vram='9216' heads='1'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </video> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 69e2612..d87654c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -342,12 +342,15 @@ mymain(void) QEMU_CAPS_BOOT_MENU, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_BOOTINDEX); DO_TEST("boot-order", false, - QEMU_CAPS_BOOTINDEX, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE); + QEMU_CAPS_BOOTINDEX, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("boot-complex", false, - QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); + QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("boot-complex-bootindex", false, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT, - QEMU_CAPS_BOOTINDEX); + QEMU_CAPS_BOOTINDEX, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("bootloader", true, QEMU_CAPS_DOMID); DO_TEST("bios", false, QEMU_CAPS_DEVICE, QEMU_CAPS_SGA); DO_TEST("clock-utc", false, NONE); @@ -365,7 +368,8 @@ mymain(void) DO_TEST("disk-many", false, NONE); DO_TEST("disk-virtio", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); DO_TEST("disk-order", false, - QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE_BOOT); + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DRIVE_BOOT, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("disk-xenvbd", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); DO_TEST("disk-drive-boot-disk", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_BOOT); @@ -433,14 +437,20 @@ mymain(void) QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("disk-ioeventfd", false, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_IOEVENTFD, - QEMU_CAPS_VIRTIO_TX_ALG, QEMU_CAPS_DEVICE); + QEMU_CAPS_VIRTIO_TX_ALG, QEMU_CAPS_DEVICE, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("disk-snapshot", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT); DO_TEST("event_idx", false, QEMU_CAPS_DRIVE, QEMU_CAPS_VIRTIO_BLK_EVENT_IDX, QEMU_CAPS_VIRTIO_NET_EVENT_IDX, - QEMU_CAPS_DEVICE); + QEMU_CAPS_DEVICE, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); + DO_TEST("virtio-lun", false, + QEMU_CAPS_DRIVE, + QEMU_CAPS_DEVICE, + QEMU_CAPS_VIRTIO_BLK_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO); DO_TEST("graphics-vnc", false, NONE); DO_TEST("graphics-vnc-socket", false, NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0a8a28e..2cc1f7d 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -190,6 +190,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("lease"); DO_TEST("event_idx"); + DO_TEST("virtio-lun"); DO_TEST("usb-redir"); DO_TEST("blkdeviotune"); -- 1.7.7.4

On 01/05/2012 01:49 PM, Eric Blake wrote:

On 01/05/2012 11:35 AM, Laine Stump wrote:

...

In the past, generic SCSI commands issued from a guest to a virtio disk were always passed through to the underlying disk by qemu, and the kernel would also pass them on.

v3 changes:

1) Add note to docs that the SCSI commands are not only accepted from the guest, but are passed through to the physical device (I didn't add anything about the SCSI commands being emulated because I still don't understand that situation perfectly, and don't want to add misleading docs about existing behavior.

2) Change the logic of qemuDomainSnapshotIsAllowed() to always return false if a device='lun' disk is encountered, regardless of the format. In reality, lun disks are always 'raw', so it would return false anyway, but this makes it clearer. Looks reasonable.

...

+++ b/docs/formatdomain.html.in @@ -1001,8 +1001,17 @@ "block", "dir", or "network" and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed - to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". The + to the guest OS. Possible values for this attribute are + "floppy", "disk", "cdrom", and "lun", defaulting to + "disk". "lun" (<span class="since">since 0.9.10</span>) is only Are we targetting 0.9.9 since this is a CVE mitigation? IIUC, the qemu default is scsi=on, so it is _only_ after this patch is applied that we override that default with scsi=off for device='disk'.

If someone upgrades their kernel for the CVE fix, then it doesn't matter. But if someone is running with the old kernel and libvirt 0.9.9, then should they get the scsi=off command line parameter by default to take advantage of the qemu mitigation that prevents SG_IO for scsi=off?

That is, I'm arguing that this patch is probably worth applying now, rather than waiting post-release, just so that someone that upgrades libvirt and qemu but not the kernel is at least less vulnerable to the CVE that was fixed by the upgraded kernel. I guess it boils down to how likely it is that someone can't afford the downtime in their host to upgrade their kernel right away, but can live with the downtime of upgrading the user-space libvirt and qemu and reboot guests to take advantage of the new scsi=off for the guest in the window while they wait for the next convenient time where the host kernel can be upgraded.

At any rate, I think we have converged; ACK whether we decide this is 0.9.9 material to be pushed now with one tweak, or 0.9.10 material to be delayed to next week.

Thanks for the reviews, Eric! Because any libvirt change would be pointless without also updating qemu (old qemu would just ignore "scsi=off" and pass through the commands anyway), I waited until after 0.9.9 was released in order to avoid any last-minute breakage. These two patches are now pushed, though.