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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list