Bug 1475250

Summary: schema for the pool field in <disk type=volume> does not accept all possible pool names
Product: Red Hat Enterprise Linux 7 Reporter: jiyan <jiyan>
Component: libvirtAssignee: John Ferlan <jferlan>
Status: CLOSED ERRATA QA Contact: Meina Li <meili>
Severity: low Docs Contact:
Priority: low    
Version: 7.4CC: dyuan, hhan, jiyan, lmen, meili, pkrempa, rbalakri, shyu, xuzhang, yisun
Target Milestone: rc   
Target Release: ---   
Hardware: x86_64   
OS: Linux   
Whiteboard:
Fixed In Version: libvirt-3.9.0-1.el7 Doc Type: No Doc Update
Doc Text:
undefined
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-04-10 10:52:40 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description jiyan 2017-07-26 09:43:43 UTC
Description of problem:
Save vm's dumpxml file after editing with special character in disk element's pool name failed while a vm can be defined and started successfully through a xml file with special character in disk element's pool name

Version-Release number of selected component (if applicable):
qemu-kvm-rhev-2.9.0-16.el7_4.3.x86_64
kernel-3.10.0-693.el7.x86_64
libvirt-3.2.0-14.el7_4.2.x86_64

How reproducible:
100%

Steps to Reproduce:
1.Prepare pool named '.test' and volume
# cat pool.xml 
<pool type='dir'>
  <name>.test</name>
  <capacity unit='bytes'>160982630400</capacity>
  <allocation unit='bytes'>63092301824</allocation>
  <available unit='bytes'>97890328576</available>
  <source>
  </source>
  <target>
    <path>/var/lib/libvirt/images2/</path>
    <permissions>
      <mode>0711</mode>
      <owner>0</owner>
      <group>0</group>
      <label>system_u:object_r:virt_image_t:s0</label>
    </permissions>
  </target>
</pool>
# virsh pool-define pool.xml 
Pool .test defined from pool.xml

# virsh pool-build .test
Pool .test built

# virsh pool-start .test
Pool .test started

# cp /var/lib/libvirt/images/RHEL-7.4-20170711.0-x86_64.qcow2 /var/lib/libvirt/images2/

# mv RHEL-7.4-20170711.0-x86_64.qcow2 vol.qcow2

# virsh pool-refresh .test
Pool .test refreshed

# virsh vol-list .test
 vol.qcow2            /var/lib/libvirt/images2/vol.qcow2

2.Prepare a xml file and the info in disk element is as following, then define the vm through the xml file and start the vm.
# cat a.xml |grep "<disk" -A 4
...
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='.test' volume='vol.qcow2'/>
      <target dev='vda' bus='virtio'/>
    </disk>
...

# virsh define a.xml 
Domain a defined from a.xml

# virsh start a
Domain a started

3.Edit another vm with the following info in disk element, and try to save it.
...
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='.test' volume='vol.qcow2'/>
      <backingStore/>
      <target dev='vda' bus='virtio'/>
      <alias name='virtio-disk0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
    </disk>
...
# virsh edit 1
error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng
Extra element devices in interleave
Element domain failed to validate content

Failed. Try again? [y,n,i,f,?]: 


Actual results:
As step 3 shows

Expected results:
In step3, Save vm's dumpxml file after editing with special character in disk element's pool name should succeed just like step2.


Additional info:

Comment 2 Jaroslav Suchanek 2017-09-07 13:54:20 UTC
I guess the pool name is limited to "[a-zA-Z0-9_\+\-]+" for ages. Just the
scheme validation is not consistent when domain/storage is being defined? Lets
see John's decision...

Comment 3 John Ferlan 2017-09-27 19:08:15 UTC
A <pool> uses :

 <define name='commonmetadata'>
    <interleave>
      <element name='name'>
        <ref name='genericName'/>
      </element>

where:

  <define name="genericName">
    <data type="string">
      <param name="pattern">[a-zA-Z0-9_\+\-]+</param>
    </data>
  </define>


and a <domain>'s <disk> syntax for a volume is:

  <define name="diskSourceVolume">
    <attribute name="type">
      <value>volume</value>
    </attribute>
    <optional>
      <element name="source">
        <attribute name="pool">
          <ref name="genericName"/>
        </attribute>

so they're using the same 'genericName'. 

The reason it fails for domain edit and not pool-edit is because the domain XML processing in qemuDomainDefineXMLFlags will cause virDomainDefParseXML to call virXMLValidateAgainstSchema in order to validate the schema. It's the only such call in libvirt, btw.

In order to see the invalid schema for the above pool.xml, one can run 'virt-xml-validate' and see that it too would fail:

# virt-xml-validate pool.xml
pool.xml:2: element name: Relax-NG validity error : Error validating datatype string
pool.xml:2: element name: Relax-NG validity error : Element name failed to validate content
pool.xml:1: element pool: Relax-NG validity error : Invalid sequence in interleave
pool.xml:1: element pool: Relax-NG validity error : Element pool failed to validate content
pool.xml fails to validate
#

If one checks the source for storagePoolDefineXML, they'll find like domains, the "name" can be anything except "\n" (uses virXMLCheckIllegalChars()). So rather than using genericName for the RNG, I'll create a 'poolName' definition to match the 'domainName' definition. That will allow the RNG to be valid.

As an aside, having a "." as the first character in the name will cause the file that gets created to be 'hidden' for "normal" directory listings. It's not a problem, just an observation.

Anyway, I've posted a patch upstream for his:

https://www.redhat.com/archives/libvir-list/2017-September/msg01037.html

Comment 4 John Ferlan 2017-10-05 12:17:37 UTC
With a slight adjustment to the original patch to add a check for the '/' character in the exclude regex (since it's not a legal character in pool name anyway), this patch has been pushed:

commit 5d7659027fdc34a042af3094d3d02a0d823272c2 (HEAD -> master, origin/master, origin/HEAD, bz1475250_v2)
Author: John Ferlan <jferlan>
Date:   Tue Oct 3 07:14:04 2017 -0400

    docs,rng: Adjust storage pool name grammar checks
    
...
    
    It's possible to define and start a pool with a '.' in the
    name; however, when trying to add a volume to a domain using
    the storage pool source with a '.' in the storage pool name,
    the domain RNG validation fails because RNG uses 'genericName'
    which does not allow a '.' in the name.
    
    Domain XML def parsing has a virXMLValidateAgainstSchema which
    generates the error. The Storage Pool XML def parsing has no
    call to virXMLValidateAgainstSchema. The only Storage Pool name
    validation occurs in virStoragePoolDefParseXML to ensure the
    name doesn't have a '/' in it and in storagePoolDefineXML to
    call virXMLCheckIllegalChars using the same parameter "\n" as
    qemuDomainDefineXMLFlags would check after the RNG check
    could be succesful.
    
    In order to resolve this, create a poolName definition in
    storagecommon.rng that will mimic the domain name regex that
    disallows a newline character, but add the "/" in the exclude
    list. Then modify the pool and volume source name definitions
    to key off that poolName.


$ git describe 5d7659027fdc34a042af3094d3d02a0d823272c2
v3.8.0-36-g5d7659027
$

Comment 6 Meina Li 2017-11-09 10:54:09 UTC
Verified on libvirt-3.9.0-1.el7.x86_64 and qemu-kvm-rhev-2.10.0-4.el7.x86_64.

Steps to verify:
1.Prepare pool named '.test' and volume.
# cat pool.xml 
<pool type='dir'>
  <name>.test</name>
  <target>
    <path>/tmp/test</path>
  </target>
</pool>
# virsh pool-define pool.xml 
Pool .test defined from pool.xml
# virsh pool-build .test
Pool .test built
# virsh pool-start .test
Pool .test started
# virsh vol-list *#
 Name                 Path                                    
------------------------------------------------------------------------------
 test.raw             /tmp/test/test.raw  
 
2. Edit a xml file and the info in disk element is as following, then save and start:
...
    <disk type='volume' device='disk'>
      <driver name='qemu' type='raw'/>
      <source pool='.test' volume='test.raw'/>
      <target dev='vdb' bus='virtio'/>
    </disk>
...
# virsh start lmn
Domain lmn started
# virsh domblklist lmn
Target     Source
------------------------------------------------
vda        /var/lib/libvirt/images/lmn.qcow2
vdb        test.raw

3.Prepare pool named '/test'.
# cat pool.xml
<pool type='dir'>
  <name>.test</name>
  <target>
    <path>/tmp/test</path>
  </target>
</pool>
# virsh pool-define pool.xml 
error: Failed to define pool from pool.xml
error: XML error: name /test cannot contain '/'

The result is as expected, so move the bug to Verified.

Comment 10 errata-xmlrpc 2018-04-10 10:52:40 UTC
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://access.redhat.com/errata/RHEA-2018:0704

Comment 11 Ján Tomko 2018-08-06 12:47:58 UTC
*** Bug 1107420 has been marked as a duplicate of this bug. ***