9:59 a.m.
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.
> + - 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".
I'd rather add that information when it actually is true (i.e. when
virtio-blk-scsi support is put in).
> 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.
Won't the list of names be different? ("sr")? At any rate, that's an
orthogonal issue, and should be addressed with a separate patch.
> + 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.
So this is a problem, but not fixable now, correct?
> 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.
Just to verify - this is another thing that you've noticed in the code,
but is orthogonal to this patch, correct?
> 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'.
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,
???