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

[Paolo Bonzini]

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