Bug 1328003

This is because there are no defaults chosen for the format for a source pool when the <disk> "type" value uses 'dir', 'network', or 'volume'.  For <disk type='file'.../> or <disk type='block'.../> the default format type is 'raw' as filled in during qemuDomainDeviceDefPostParse.   Naturally, each of the types without a default were added after the initial implementation.

For certain code paths, if you modify /etc/libvirt/qemu.conf and change/set the entry "allow_disk_format_probing" to 1, then libvirt will attempt to automatically determine the format type (although that comes with some dire warnings regarding being a security hole).

The code path for snapshot; however, does not do any sort of probing or defaulting setting of the source format type.

Obviously, the best answer is to always provide a 'type' field because well you know best.  Letting libvirt "decide upon" a default or "probe" for a default. I could certainly "reason" that for 'dir' and perhaps 'volume' using "mode='host'" that perhaps libvirt could also use 'raw' for a default, but whether that's the "best answer" for all cases is a different issue.

Leaving needinfo for pkrempa since he knows more about the details of snapshots - in qemuDomainSnapshotPrepare, the check that's failing for scenario2:

    if (vm->def->disks[i]->src->format > 0 &&
        vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                       _("internal snapshot for disk %s unsupported "
                         "for storage type %s"),


Why is "> 0" (although it probably should have been: > VIR_STORAGE_FILE_NONE)?  As is shown by scenario1, that means UNKNOWN is "ok" and that's being allowed.

Second question - if we find NONE, would it be (ahem) wise or desirable to attempt a call to virStorageFileGetMetadata() in order to get that data filled in?  Especially since virStorageTranslateDiskSourcePool doesn't fill in a disk->source->format value. I see hotplug (ChangeDisk, AttachDeviceDisk) has made a call to qemuDomainDetermineDiskChain - is that perhaps a better option?

Alternatively, modifying qemuDomainDeviceDefPostParse to fill in "defaults" for DIR and a VOLUME with a SRCPOOL using "host" mode, eg.:

            if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_NONE &&
                (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE ||
                 virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_BLOCK ||
                  virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_DIR ||
                 (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_VOLUME &&
                  disk->src->srcpool &&
                  disk->src->srcpool->mode ==
                  VIR_STORAGE_SOURCE_POOL_MODE_HOST)))
                virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
...

I can reason filling in src->format perhaps during the Prepare code, but that only solves things for disk->src>-type == VOLUME...  Looks like perhaps the answer might be the call to qemuDomainDetermineDiskChain after Prepare returns successfully in at least the PrepareInternal case?  It seems the external case will cause an disk->src->format of type NONE to default to "QCOW2", so that's covered.  I just don't know the code well enough - so I figured I'd ask first...

(In reply to John Ferlan from comment #1)
> This is because there are no defaults chosen for the format for a source
> pool when the <disk> "type" value uses 'dir', 'network', or 'volume'.  For
> <disk type='file'.../> or <disk type='block'.../> the default format type is
> 'raw' as filled in during qemuDomainDeviceDefPostParse.   Naturally, each of
> the types without a default were added after the initial implementation.

I think that the right approach would be that the volume to storage source translator should fill the type from the libvirt storage pool if possible. Otherwise we should probably use the raw type as with other disks. 

> For certain code paths, if you modify /etc/libvirt/qemu.conf and change/set
> the entry "allow_disk_format_probing" to 1, then libvirt will attempt to
> automatically determine the format type (although that comes with some dire
> warnings regarding being a security hole).
> 
> The code path for snapshot; however, does not do any sort of probing or
> defaulting setting of the source format type.

It's not necessary or desired for internal snapshots. The format should either be provided by the user or probed if that is allowed. Anything other is not desired.

> Obviously, the best answer is to always provide a 'type' field because well
> you know best.  Letting libvirt "decide upon" a default or "probe" for a
> default. I could certainly "reason" that for 'dir' and perhaps 'volume'
> using "mode='host'" that perhaps libvirt could also use 'raw' for a default,
> but whether that's the "best answer" for all cases is a different issue.

In cases where format probing is disabled and we did not get any format from the user we should choose "raw", otherwise you are opening the security hole of format detection in qemu.

> Leaving needinfo for pkrempa since he knows more about the details of
> snapshots - in qemuDomainSnapshotPrepare, the check that's failing for
> scenario2:
> 
>     if (vm->def->disks[i]->src->format > 0 &&
>         vm->def->disks[i]->src->format != VIR_STORAGE_FILE_QCOW2) {
>         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                        _("internal snapshot for disk %s unsupported "
>                          "for storage type %s"),
> 
> 
> Why is "> 0" (although it probably should have been: >
> VIR_STORAGE_FILE_NONE)?  As is shown by scenario1, that means UNKNOWN is

Since we declare 'VIR_STORAGE_FILE_NONE = 0' the two cases you describe are equivalent.

> "ok" and that's being allowed.
> 
> Second question - if we find NONE, would it be (ahem) wise or desirable to
> attempt a call to virStorageFileGetMetadata() in order to get that data
> filled in?  Especially since virStorageTranslateDiskSourcePool doesn't fill

No. Especially not when it's disabled in the config file.

> in a disk->source->format value. I see hotplug (ChangeDisk,

Well if it knows the format it should probably fill it. In other cases we should treat it as raw. Except if the user provides the format explicitly.

> AttachDeviceDisk) has made a call to qemuDomainDetermineDiskChain - is that
> perhaps a better option?

That wouldn't help much:
1) It was already called on domain disks on startup
2) if format detection is disabled it won't do anything reasonable.
3) the function skips format checking a determination for storage technologies that we can't access

> Alternatively, modifying qemuDomainDeviceDefPostParse to fill in "defaults"
> for DIR and a VOLUME with a SRCPOOL using "host" mode, eg.:
> 
>             if (virDomainDiskGetFormat(disk) == VIR_STORAGE_FILE_NONE &&
>                 (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_FILE ||
>                  virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_BLOCK ||
>                   virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_DIR ||
>                  (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_VOLUME &&
>                   disk->src->srcpool &&
>                   disk->src->srcpool->mode ==
>                   VIR_STORAGE_SOURCE_POOL_MODE_HOST)))

The pool mode is relevant just for iSCSI, this should actually work for all pool types.

>                 virDomainDiskSetFormat(disk, VIR_STORAGE_FILE_RAW);
> ...
> 
> I can reason filling in src->format perhaps during the Prepare code, but
> that only solves things for disk->src>-type == VOLUME...  Looks like perhaps
> the answer might be the call to qemuDomainDetermineDiskChain after Prepare
> returns successfully in at least the PrepareInternal case?  It seems the
> external case will cause an disk->src->format of type NONE to default to
> "QCOW2", so that's covered.  I just don't know the code well enough - so I
> figured I'd ask first...

I think the snapshot code is not the problem here. We should correctly fill the format of the volume at startup/translation and then the snapshot code will just work.

The point about "0" comparison wasn't that it wasn't equivalent - the point was we shouldn't be using a magic number "0" when the symbol is available. Makes it easier to later search on uses of VIR_STORAGE_FILE_NONE or VIR_STORAGE_FILE_.

Still not 100% convinced the snapshot code should be allowing > 0...  If that format field is 0, then that's not good I think (or at least how I read this bug). Regardless of whether the volume translation code isn't filling it in. I do see that "somehow" the network devices get raw (even though it could be argued they shouldn't).  I can take the same target "file" for my iSCSI pool and if I present as "type='network' device='disk'", then somehow the format gets set to "raw"; however, the same device looked at through the storage pool sees "dos" as the format.  The same device presented as "type='volume' device='disk'" will show up as NONE (as seen in this bug).

So I'm not quite convinced current algorithms to translate the network and/or volume source pool format types are getting the right answer.  Especially since a volume in a pool doesn't necessarily use the virStorageFileFormatType to describe the volume target format type.  

I'm chasing the conversions now and will figure this out, but for starters... As I dig on this I'm finding SCSI/iSCSI pool volumes use the "Disk" volume format types which won't translate well and for some shouldn't translate to the disks[i]->src->format because they're not "virStorageFileFormatType"'s.

The following is taken from the virStoragePoolTypeInfo poolTypeInfo table.

           Default      
Pool       Vol Format   Format Table Type "decoder"
---------------------
Logical    NONE       NONE
Dir        NONE       NONE
FS         RAW        virStorageFileFormatType
NetFS      RAW        virStorageFileFormatType
iSCSI      NONE       virStoragePoolFormatDiskType
SCSI       NONE       virStoragePoolFormatDiskType
RBD        RAW        virStorageFileFormatType
Sheepdog   RAW        virStoragePoolFormatDiskType   <--??
Gluster    RAW        virStorageFileFormatType
Mpath      NONE       virStoragePoolFormatDiskType
Disk       NONE       virStoragePoolFormatDiskType
ZFS        NONE       NONE

Don't know enough about Sheepdog, but since it cannot (yet) be used to translate a volume to a disk, then it probably doesn't matter.  It can be used for a Network Disk though...

So it seems the "magic" happens during qemuDomainCheckDiskPresence and the determination of whether to call qemuDomainDetermineDiskChain which will call virStorageFileGetMetadata to fill in the data.

Using the same "disk" defined 3 different ways:

A:
<disk type='network' device='disk'>
  <driver name='qemu'/>
  <source protocol='iscsi' name='iqn.2013-12.com.example:iscsi-chap-netpool/1'>
    <host name='192.168.122.1' port='3260'/>
  </source>
  <target dev='vdb' bus='virtio'/>
  <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
</disk>

B:
<disk type='volume' device='disk'>
  <driver name='qemu'/>
  <source pool='iscsi-net-pool' volume='unit:0:0:1' mode='direct'/>
  <target dev='vdb' bus='virtio'/>
  <alias name='virtio-disk1'/>
  <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
</disk>

C:
<disk type='volume' device='disk'>
  <driver name='qemu'/>
  <source pool='iscsi-net-pool' volume='unit:0:0:1' mode='host'/>
  <target dev='vdb' bus='virtio'/>
  <alias name='virtio-disk1'/>
  <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
</disk>


For A & B, when the domain starts qemuDomainDetermineDiskChain will be called and the driver format will be determined to be 'raw' when virStorageFileGetMetadata is called because we're not probing and the default is to set to VIR_STORAGE_FILE_RAW.

For C, when the domain starts qemuDomainDetermineDiskChain does not get called because virStorageSourceIsLocalStorage finds the 'actualtype' of NETWORK. So the total reliance is on what virStorageTranslateDiskSourcePool has set for the format.  

So yes, as Peter notes because the VIR_STORAGE_SOURCE_POOL_MODE_HOST is only used for iSCSI as long as it fills in the format as RAW, then other code will just work. The "host" mode was added to access a 'local' LUN via it's /dev/disk/by-path/* path rather than via an iSCSI network connection (if the iSCSI service is on the current host, then this works).

Still the question lingers, is filling with RAW what's expected? Or should this code also make the qemuDomainDetermineDiskChain call which will either set the type to raw or (allow for) the probing (if desired).

A secondary question would be should check:

            format >= VIR_STORAGE_FILE_NONE &&
            format < VIR_STORAGE_FILE_BACKING &&

in qemuDomainCheckDiskPresence to help decide whether to fall into the call for qemuDomainDetermineDiskChain be changed to:

            format > VIR_STORAGE_FILE_NONE &&
            format < VIR_STORAGE_FILE_BACKING &&

That is - if we're local and the file exists, BUT we don't have a format, then be sure to get that format.

I'll see what the upstream community has to say with some patches...




Other pool types that

(In reply to John Ferlan from comment #3)
> The point about "0" comparison wasn't that it wasn't equivalent - the point
> was we shouldn't be using a magic number "0" when the symbol is available.
> Makes it easier to later search on uses of VIR_STORAGE_FILE_NONE or
> VIR_STORAGE_FILE_.

I definitely didn't think that you meant to discuss coding style rather an actual problem in the code which lead me to look through the code.

> Still not 100% convinced the snapshot code should be allowing > 0...  If
> that format field is 0, then that's not good I think (or at least how I read

That is still a manifestation of the bug. The real problem is that the format isn't selected. We probably can make the snapshot code more robust though.

> this bug). Regardless of whether the volume translation code isn't filling
> it in. I do see that "somehow" the network devices get raw (even though it
> could be argued they shouldn't).  I can take the same target "file" for my

From qemu's point of view it's irrelevant what the source is. A qcow can be placed on a partition, file or network storage device, thus networked storage requires a format too. I don't see how 'raw' should not be a default for networked storage.

> iSCSI pool and if I present as "type='network' device='disk'", then somehow
> the format gets set to "raw"; however, the same device looked at through the
> storage pool sees "dos" as the format.  The same device presented as
> "type='volume' device='disk'" will show up as NONE (as seen in this bug).

Yes, the default doesn't get set and the translation code doesn't set it either.

> So I'm not quite convinced current algorithms to translate the network
> and/or volume source pool format types are getting the right answer. 

Yes that is the problem.

> Especially since a volume in a pool doesn't necessarily use the
> virStorageFileFormatType to describe the volume target format type.  

That depends in the end on the fact whether we are actually able to determine the type from the volume itself.

> I'm chasing the conversions now and will figure this out, but for
> starters... As I dig on this I'm finding SCSI/iSCSI pool volumes use the
> "Disk" volume format types which won't translate well and for some shouldn't
> translate to the disks[i]->src->format because they're not
> "virStorageFileFormatType"'s.

Yes. Partition table format is totally irrelevant in this aspect. Even virStorageVolFormatDisk which is a little bit more relevant since it's a type of the volume itself doesn't really translate well. In addition we should not probe the type. Thus raw should be used unless the user specified anything else.

> 
> The following is taken from the virStoragePoolTypeInfo poolTypeInfo table.
> 
>            Default      
> Pool       Vol Format   Format Table Type "decoder"
> ---------------------
> Logical    NONE       NONE
> Dir        NONE       NONE
> FS         RAW        virStorageFileFormatType
> NetFS      RAW        virStorageFileFormatType
> iSCSI      NONE       virStoragePoolFormatDiskType
> SCSI       NONE       virStoragePoolFormatDiskType
> RBD        RAW        virStorageFileFormatType
> Sheepdog   RAW        virStoragePoolFormatDiskType   <--??
> Gluster    RAW        virStorageFileFormatType
> Mpath      NONE       virStoragePoolFormatDiskType
> Disk       NONE       virStoragePoolFormatDiskType
> ZFS        NONE       NONE
> 
> Don't know enough about Sheepdog, but since it cannot (yet) be used to
> translate a volume to a disk, then it probably doesn't matter.  It can be
> used for a Network Disk though...

A better question is whether the volume itself can contain a qcow or anything similar which allows backing files. If yes and it is obvious or can be specified at volume creation time, then we should take that into account. Otherwise there's only one sane option and that is to default to 'raw'.

(In reply to John Ferlan from comment #4)
> So it seems the "magic" happens during qemuDomainCheckDiskPresence and the
> determination of whether to call qemuDomainDetermineDiskChain which will
> call virStorageFileGetMetadata to fill in the data.
> 
> Using the same "disk" defined 3 different ways:
> 
> A:
> <disk type='network' device='disk'>
>   <driver name='qemu'/>
>   <source protocol='iscsi'
> name='iqn.2013-12.com.example:iscsi-chap-netpool/1'>
>     <host name='192.168.122.1' port='3260'/>
>   </source>
>   <target dev='vdb' bus='virtio'/>
>   <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
> </disk>
> 
> B:
> <disk type='volume' device='disk'>
>   <driver name='qemu'/>
>   <source pool='iscsi-net-pool' volume='unit:0:0:1' mode='direct'/>
>   <target dev='vdb' bus='virtio'/>
>   <alias name='virtio-disk1'/>
>   <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
> </disk>
> 
> C:
> <disk type='volume' device='disk'>
>   <driver name='qemu'/>
>   <source pool='iscsi-net-pool' volume='unit:0:0:1' mode='host'/>
>   <target dev='vdb' bus='virtio'/>
>   <alias name='virtio-disk1'/>
>   <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
> </disk>
> 
> 
> For A & B, when the domain starts qemuDomainDetermineDiskChain will be
> called and the driver format will be determined to be 'raw' when
> virStorageFileGetMetadata is called because we're not probing and the
> default is to set to VIR_STORAGE_FILE_RAW.
> 
> For C, when the domain starts qemuDomainDetermineDiskChain does not get
> called because virStorageSourceIsLocalStorage finds the 'actualtype' of
> NETWORK. So the total reliance is on what virStorageTranslateDiskSourcePool
> has set for the format. 

This paragraph is not acurate. In cases A and B virStorageSourceIsLocalStorage returns _false_ and thus the short-circuit code is not called so we call qemuDomainDetermineDiskChain.

For the third case the type actually is translated as BLOCK and thus the short circuit code is called.

So the problem is a combination of the following factors:

1) we don't set a default type when parsing the disk for disk type='volume' as we might be able to determine it from the volume itself
2) the translation code doesn't actually set the type since it can't determine it, but sets type=BLOCK and the storage path accurately
3) qemuDomainCheckDiskPresence has the short circuit code to avoid calling the backing store crawler on files that don't have backing store
4) there is a bug in the condition that determines whether a file has backing chain


> So yes, as Peter notes because the VIR_STORAGE_SOURCE_POOL_MODE_HOST is only
> used for iSCSI as long as it fills in the format as RAW, then other code
> will just work. The "host" mode was added to access a 'local' LUN via it's
> /dev/disk/by-path/* path rather than via an iSCSI network connection (if the
> iSCSI service is on the current host, then this works).
> 
> Still the question lingers, is filling with RAW what's expected? Or should

Yes. The user needs to provide the type to use anything besides raw if we can't know better.

> this code also make the qemuDomainDetermineDiskChain call which will either
> set the type to raw or (allow for) the probing (if desired).

Yes, the bug is in the single equals sign in the short-circuit code.

Fixed upstream:

commit a391a9c5b1cfb057af734928ded0c0b0900ef41f
Author: Peter Krempa <pkrempa>
Date:   Thu May 5 15:25:52 2016 +0200

    qemu: domain: Don't treat unknown storage type as not having backing chain
    
    qemuDomainCheckDiskPresence has short-circuit code to skip the
    determination of the disk backing chain for storage formats that can't
    have backing volumes. The code treats VIR_STORAGE_FILE_NONE as not
    having backing chain and skips the call to qemuDomainDetermineDiskChain.
    
    This is wrong as qemuDomainDetermineDiskChain is responsible for storage
    format detection and has logic to determine the default type if format
    detection is disabled.
    
    This allows to storage passed via <disk type="volume"> to circumvent the
    enforcement to have correct storage format or that we shall default to
    format='raw', since we don't set the default type via the post parse
    callback for "volume" backed disks as the translation code could come up
    with a better guess.

Verified version 
libvirt-2.0.0-5.el7.x86_64

Verified steps: 
If I understand all above discussion correctly,
For volume disk, libvirt will set disk type to raw as default if user 
does not provide it and without disk type probing.

1. define a guest and prepare a iscsi volume disk xml as following without disk type
# cat vol-disk-iscsi.xml 
<disk type='volume' device='disk'>
	<driver name='qemu'/>
	<source mode='host' pool='iscsi-pool' volume='unit:0:0:1'/>
	<target dev='vde' bus='virtio'/>
</disk>

2. cold-plug this volume disk into guest

# virsh attach-device r73 vol-disk.xml --persistent
Device attached successfully

3. dumpxml to check it will set format to raw as default 
# virsh dumpxml r73 | grep disk -A 9
   .....
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='iscsi-pool' volume='unit:0:0:1' mode='host'/>
      <backingStore/>
      <target dev='vde' bus='virtio'/>
      <alias name='virtio-disk4'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </disk>

try to create snapshots, it should fail to create.

# virsh snapshot-create-as r73 s1
error: unsupported configuration: internal snapshot for disk vde unsupported for storage type raw

4. Test logical volume disk, default pool as above, it can add default format raw if we don't provide after guest start.

Hi Peter,
I think steps in above comment can be used to verify this bug, but I still has a concern of the following scenario.I am a little confused about that.
Would you please help check the result is expected or not,
Thanks a lot in advance!

Issue I met : 
If I enable disk format probing in qemu.conf, attaching direct mode iscsi volume disk without source type, after starting guest, it still has no type in xml. 

If I disable disk format probing in qemu.conf, attaching direct mode iscsi volume disk without source type, after starting guest, it will fill type='raw' as default. 

Details as following :
1. enable disk format probing in qemu.conf

# cat /etc/libvirt/qemu.conf | grep probing
......
allow_disk_format_probing = 1

2. restart libvirtd
#systemctl restart libvirtd

3. attaching a iscsi volume disk with direct mode but no source type define.
check guest xml as following : 
# virsh dumpxml r73 | grep disk -A 9

    <disk type='volume' device='disk'>
      <driver name='qemu'/>
      <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>
      <target dev='vde' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </disk>

4.start guest chek again 


# virsh start r73
Domain r73 started

# virsh dumpxml r73 | grep disk -A 9
......
   <disk type='volume' device='disk'>
      <driver name='qemu'/>
      <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>
      <backingStore/>
      <target dev='vde' bus='virtio'/>
      <alias name='virtio-disk4'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </disk>

5. create snapshot
# virsh snapshot-create-as r73 s1
Domain snapshot s1 created
# virsh snapshot-list r73
 Name                 Creation Time             State
------------------------------------------------------------
 s1                   2016-08-19 15:11:35 +0800 running


6. disable disk format probing in qemu.conf, restart libvirtd
destroy and start guest again, check it will fill type='raw' 

# virsh destroy r73; virsh start r73
Domain r73 destroyed

Domain r73 started

# virsh dumpxml r73 | grep disk -A 9
    ......
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>
      <backingStore/>
      <target dev='vde' bus='virtio'/>
      <alias name='virtio-disk4'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </disk>

> Details as following :
> 1. enable disk format probing in qemu.conf
> 
> # cat /etc/libvirt/qemu.conf | grep probing
> ......
> allow_disk_format_probing = 1
> 
> 2. restart libvirtd
> #systemctl restart libvirtd
> 
> 3. attaching a iscsi volume disk with direct mode but no source type define.
> check guest xml as following : 
> # virsh dumpxml r73 | grep disk -A 9
> 
>     <disk type='volume' device='disk'>
>       <driver name='qemu'/>
>       <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>
>       <target dev='vde' bus='virtio'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x09'
> function='0x0'/>
>     </disk>
> 
> 4.start guest chek again 
> 
> 
> # virsh start r73
> Domain r73 started
> 
> # virsh dumpxml r73 | grep disk -A 9
> ......
>    <disk type='volume' device='disk'>
>       <driver name='qemu'/>
>       <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>

Libvirt unfortunately doesn't have code in place to allow format probing on SCSI volumes in direct mode and thus does not fill in any type.

>       <backingStore/>
>       <target dev='vde' bus='virtio'/>
>       <alias name='virtio-disk4'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x09'
> function='0x0'/>
>     </disk>
> 
> 5. create snapshot
> # virsh snapshot-create-as r73 s1
> Domain snapshot s1 created
> # virsh snapshot-list r73
>  Name                 Creation Time             State
> ------------------------------------------------------------
>  s1                   2016-08-19 15:11:35 +0800 running

So this in fact was a qcow2 file.

> 
> 
> 6. disable disk format probing in qemu.conf, restart libvirtd
> destroy and start guest again, check it will fill type='raw' 
> 
> # virsh destroy r73; virsh start r73
> Domain r73 destroyed
> 
> Domain r73 started
> 
> # virsh dumpxml r73 | grep disk -A 9
>     ......
>     <disk type='volume' device='disk'>
>       <driver name='qemu' type='raw'/>
>       <source pool='iscsi-pool' volume='unit:0:0:1' mode='direct'/>

This is expected, no format is specified thus raw was added if format probing is disabled.

>       <backingStore/>
>       <target dev='vde' bus='virtio'/>
>       <alias name='virtio-disk4'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x09'
> function='0x0'/>
>     </disk>

Both cases listed above are expected. The first case is present as libvirt can't probe the volume despite it being enabled.

Thanks a lot for your info. 
According to comment 9, move this bug to verified.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2016-2577.html