[libvirt] [PATCH] scsi: Need to translate disk source pool in config attach path

[John Ferlan]

On 06/12/2015 07:52 AM, Erik Skultety wrote:
> 
> 
> On 06/12/2015 01:10 PM, John Ferlan wrote:
>>
>>
>> On 06/12/2015 04:57 AM, Erik Skultety wrote:
>>>
>>>
>>> On 06/11/2015 11:15 PM, John Ferlan wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1228007
>>>>
>>>> When attaching a scsi volume lun via the attach-device --config or
>>>> --persistent options, there was no translation of the source pool
>>>> like there was for the live path, thus the attempt to modify the config
>>>> would fail since not enough was known about the disk.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  src/qemu/qemu_driver.c | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 34e5581..6bb8549 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -8016,7 +8016,8 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn,
>>>>  static int
>>>>  qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
>>>>                               virDomainDefPtr vmdef,
>>>> -                             virDomainDeviceDefPtr dev)
>>>> +                             virDomainDeviceDefPtr dev,
>>>> +                             virConnectPtr conn)
>>>>  {
>>>>      virDomainDiskDefPtr disk;
>>>>      virDomainNetDefPtr net;
>>>> @@ -8033,6 +8034,8 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
>>>>                             _("target %s already exists"), disk->dst);
>>>>              return -1;
>>>>          }
>>>> +        if (virStorageTranslateDiskSourcePool(conn, disk) < 0)
>>>> +            return -1;
>>>>          if (qemuCheckDiskConfig(disk) < 0)
>>>>              return -1;
>>>>          if (virDomainDiskInsert(vmdef, disk))
>>>> @@ -8501,7 +8504,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>>>>                                           VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
>>>>              goto endjob;
>>>>  
>>>> -        if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0)
>>>> +        if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev,
>>>> +                                                dom->conn)) < 0)
>>>>              goto endjob;
>>>>      }
>>>>  
>>>>
>>> I think we should also update the docs in the way that we also support
>>> storage type 'volume' with disk device type 'lun'. And that is because
>>> we do not really care about the storage type when performing a device
>>> attach (we do care about this when doing snapshots), yet we still need
>>> to call virStorageDiskSourcePool which returns instantly for every
>>> storage type, except for volume. Documentation update should also imply
>>> updating our RNG schema (domaincommon I think), otherwise the use case
>>> described in the BZ won't validate. ACK if you also update both docs and
>>> RNG.
>>>
>>
>> Not sure I understand what the RNG updated requirement is... The RNG
>> already has all the proper formats defined when it was added in commit
>> id 'c00b2f0d' (if you use gitk you can see other similar commits around
>> that time as well).
>>
> What I meant is: "Using 'lun' (since 0.9.10) is only valid when type is
> 'block' or 'network' using iSCSI protocol..." should be updated to allow
> the source type 'volume' as well, to reflect the nice paragraph below.
> If you look at the BZ, the device definitions begins with:
> <disk type='volume' device='lun'> ===> that device is parsed and
> processed correctly and also will be attached successfully later on
> (provided that your patch is merged). However it fails the RNG
> validation right here (domaincommon.rng):
> <attribute name="device">
>   <value>lun</value>
> </attribute>
> <optional>
>   <ref name="rawIO"/>
> </optional>
> <optional>
>   <ref name="sgIO"/>
> </optional>
> <interleave>
>   <choice>
>     <ref name="diskSourceNetwork"/>
>     <ref name="diskSourceBlock"/>	<<<< diskSourceVolume missing
>   </choice>
>   <ref name="diskSpecsExtra"/>
> </interleave>
> 
> 
>> Using a LUN from an iSCSI source pool provides the same capabilities as
>> a <code>disk</code> configured using <code>type</code> 'block' or
>> 'network and <code>device</code> of 'lun' with respect to how the LUN is
>> presented to and may used by the guest.
>>
> ^^^ This one's nice.
>>
>> Possible replacement for "capabilities" - "features"? Or some other
>> suggestion.
> I like 'features', but that's just a matter of personal preference, so
> do as you wish.

OK - I'll squash the attached in

Thanks -

John

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-issues.patch
Type: text/x-patch
Size: 2436 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150612/6767dd5b/attachment-0001.bin>