Re: [libvirt] [PATCHv2 2/2] qemu: add new disk device='lun' for bus='virtio' & type='block'

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).

> 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?

> 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?

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,