Bug 1540290

+++ This bug was initially created as a clone of Bug #1534396 +++

Description of problem:
The iscsi lun number in source name will be changed after guest restart with external snapshot.

Version-Release number of selected component (if applicable):
libvirt-3.9.0-7.el7.x86_64
qemu-kvm-rhev-2.10.0-15.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Prepare a guest with iscsi network disk.
# virsh dumpxml rhel7 | grep disk -a6
... <disk type='network' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
       <host name='**IP**' port='3260'/>
       <auth username='redhat'>
         <secret type='iscsi' usage='libvirtiscsi'/>
       </auth>
     </source>
     <target dev='vda' bus='virtio'/>
     <alias name='virtio-disk0'/>
     <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
   </disk>...

2. Create external disk and check xml.
# for i in 1 2 3 4;do virsh snapshot-create-as rhel7 s$i --disk-only --diskspec vda,file=/tmp/rhel7.s$i;done
# virsh dumpxml rhel7 | grep disk -a12 
... <disk type='file' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source file='/tmp/rhel7.s4'/>
     <backingStore type='file' index='1'>
       <format type='qcow2'/>
       <source file='/tmp/rhel7.s3'/>
       <backingStore type='file' index='2'>
         <format type='qcow2'/>
         <source file='/tmp/rhel7.s2'/>
         <backingStore type='file' index='3'>
          <format type='qcow2'/>
           <source file='/tmp/rhel7.s1'/>
           <backingStore type='network' index='4'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
               <host name='**IP**' port='3260'/>
             </source>
           </backingStore>...

3. Restart guest and check guest xml.
# virsh destroy rhel7
# virsh start rhel7
# virsh dumpxml rhel7 | grep disk -a12
... <disk type='file' device='disk'>
     <driver name='qemu' type='qcow2'/>
     <source file='/tmp/rhel7.s4'/>
         <backingStore type='file' index='2'>
          <format type='qcow2'/>
           <source file='/tmp/rhel7.s1'/>
           <backingStore type='network' index='3'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/0'>     
               <host name='**IP**' port='3260'/>
             </source>                                                    --
           </backingStore>...

Actual results:
As above step 3.

Expected results:
# virsh dumpxml rhel7 | grep disk -a12
… <backingStore type='network' index='2'>
            <format type='qcow2'/>
             <source protocol='iscsi' name='iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6/2'>
               <host name='**IP**' port='3260'/>
             </source>
           </backingStore>...

Additional info:
After blockcommit/blockpull/blockcopy, it also will be changed.

--- Additional comment from Peter Krempa on 2018-01-25 13:49:02 CET ---

Could you please post output of 'qemu-img info /tmp/rhel7.s1' when that reproduces? it looks like we parse something wrong from the file on the disk

--- Additional comment from Meina Li on 2018-01-26 03:26:37 CET ---

After restart guest, check the info:
# qemu-img info /tmp/lmn.s1 -U  --backing-chain
image: /tmp/lmn.s1
file format: qcow2
virtual size: 5.0G (5368709120 bytes)
disk size: 196K
cluster_size: 65536
backing file: json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}
backing file format: qcow2
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

image: json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}
file format: qcow2
virtual size: 5.0G (5368709120 bytes)
disk size: unavailable
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false

--- Additional comment from Peter Krempa on 2018-01-29 15:39:38 CET ---

Thanks for the information.

In the backing json string the 'lun' field is clearly a string:

json:{"driver": "qcow2", "file": {"lun": "2", "portal": "**IP**:3260", "driver": "iscsi", "transport": "tcp", "target": "iqn.2003-01.org.linux-iscsi.localhost.x8664:sn.9cba196611e6"}}

while the documentation in qemu (qapi/block-core.json) clearly states that 'lun' is an int:

{ 'struct': 'BlockdevOptionsIscsi',
  'data': { 'transport': 'IscsiTransport',
            'portal': 'str',
            'target': 'str',
            '*lun': 'int',
            '*user': 'str',
            '*password-secret': 'str',
            '*initiator-name': 'str',
            '*header-digest': 'IscsiHeaderDigest',
            '*timeout': 'int' } }


Libvirt is parsing it as an int and thus fails to find it and uses the default (0).


--- Additional comment from Kevin Wolf on 2018-01-30 16:45:57 CET ---

Well, nobody told libvirt to parse this string or that it matches the BlockdevOptionsIscsi structure... qemu accepts both strings and integers in json: strings because internally everything is converted into strings anyway (we're in the -drive legacy code path here, so no QAPI schema involved at all). Consequently, the generated json: strings treat all values as strings, too.

Maybe this can be improved by going through a real QAPI object using the keyval visitor, if we can assume that the content always actually contains everything that the schema requires. In that case you'd actually get JSON numbers for integers in the schema.

If that doesn't work and we actually need to use QAPI objects internally (which is something that we want, but it's really hard), don't hold your breath. It might be a few years then.

Either way, if libvirt parses the strings, it should probably accept both integers and strings as we won't be able to retroactively change all qemu versions from 2.1 where the feature was introduced.

--- Additional comment from Peter Krempa on 2018-01-30 18:25:53 CET ---

(In reply to Kevin Wolf from comment #7)
> Well, nobody told libvirt to parse this string or that it matches the
> BlockdevOptionsIscsi structure... qemu accepts both strings and integers in
> json: strings because internally everything is converted into strings anyway
> (we're in the -drive legacy code path here, so no QAPI schema involved at
> all). Consequently, the generated json: strings treat all values as strings,
> too.

Well the fact that we parse it is a result of qemu starting to put those strings into the backing store field in the qcow images. Libvirt uses those to reconstruct the backing chain and since there's no other documentation than the QAPI schema it makes sense that people will use it.

Okay I'll clone this BZ to fix the JSON parsers to accept both strings and numbers for the 'int' fields.