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.
+ - 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).
More precisely, when support for device='lun' is added for SCSI disks (and again, it will work also for other controllers, not just virtio-scsi). So that's fine to leave it as is.
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?
Yes.
Side note: this fails if you're trying to hot-plug a SCSI CD drive. Probably this error condition in qemuDomainChangeEjectableMedia:
[snip]
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?
Yes.
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,
Makes sense. Paolo