On Wed, Dec 23, 2020 at 14:28:04 -0600, Ryan Gahagan wrote: > On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa <pkrempa@xxxxxxxxxx> wrote: > > > For this test, > > you'll be better off adding a hack to fill in the uid/gid values (e.g. > > by checking that they start with a + to be safe. > > > Our interpretation of what you mean here is to add a check in our parse > method (specifically in virDomainStorageNetworkParseNFS, which is called by > virDomainDIskSourceNetworkParse in domain_conf.c) which looks at the > user/group strings in the XML and checks if their first character is a '+'. > If so, assume that this string is an integer and not a name, then parse it > into the nfs_uid or gid values. This would allow our tests to understand a > property like "user='+2'" without having to call the preparseStorageSource > methods. This functionality is built into virGetUserID, but because we > can't call it we would have to duplicate the code. Is this an acceptable > approach? TEST_JSON_FORMAT_NET does an XML->JSON->XML coversion to see whether the back-and-forth conversion works, but that's not really entirely necessary. The test was added so that the JSON bit can be validated, but meanwhile we do that also in the qemuxml2argvtest. You can instead try a simpler method. Add a TEST_BACKING_PARSE case into tests/virstoragetest.c That tests just JSON->XML so has fewer moving parts. Since the qemuxml2argv test case is mandatory the second part will be covered regardless. > > The formatter omits the values if they are default. According to the QMP > > schema they can be missing. The parser thus must cope when the JSON > > object doesn't have the values populated. > > > > So will a method like "virJSONValueObjectGetNumberInt" return an error code > (i.e. < 0) if the property is missing? If so, we could use this error code > as an indicator that the property (e.g. user) should be the default value > (e.g. -1). > > We also had some questions in regards to other tests. We won't submit it as > an RFC patch because it isn't ready, but if you'd like to see our current > code for reference you can view it on this branch > <https://gitlab.com/bschoney/libvirt/-/tree/issue-90-rt2>. > > We're failing virschematest for our file disk-network-nfs.xml. The error is: > "Extra element devices in interleave > Element domain failed to validate content" You can use the 'jing' validator for a more expressive output: $ jing /home/pipo/libvirt/docs/schemas/domain.rng /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:29:55: error: attribute "protocol" not allowed here; expected attribute "file", "index" or "startupPolicy" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:29:55: error: attribute "name" not allowed here; expected attribute "file", "index" or "startupPolicy" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:30:56: error: element "host" not allowed here; expected the element end-tag or element "encryption", "seclabel" or "slices" /home/pipo/libvirt/tests/qemuxml2argvdata/disk-network-nfs.xml:33:29: error: value of attribute "type" is invalid; must be equal to "bochs", "cloop", "cow", "dir", "dmg", "fat", "iso", "luks", "ploop", "qcow", "qcow2", "qed", "raw", "vdi", "vhd", "vmdk" or "vpc" > This seems like there's a problem with our XML not matching the RNG docs. > However, the problem appears to be with our <devices> tag, which was based > on the XML in disk-backing-chains.xml. Why does our file report an error > for this, but disk-backing-chains.xml does not? The problem is that the definition of the second disk in 'disk-network-nfs' is plainly broken: <disk> <driver name='qemu' type='qcow2'/> <source protocol='gluster' name='Volume2/Image'> <host transport='unix' socket='/path/to/sock'/> </source> <backingStore type='network' index='1'> <format type='nfs'/> <source protocol='nfs' name='/backing/store/nfs'> <host name='example.org'/> <nfs user='USER' group='GROUP'/> </source> <backingStore/> </backingStore> <target dev='vda' bus='virtio'/> </disk> Firstly 'type' and 'device' are mandatory attributes for the <disk> element. Also you've got two disks with <target dev='vda'> but the target must be unique. Also <format type='nfs'> is wrong. The format is raw/qcow2/etc After fixing all of those I'm getting 184) QEMU XML-2-ARGV disk-network-nfs.x86_64-latest ... libvirt: error : invalid argument: Failed to parse user 'USER' ALL of the above was possible to debug even without the XML schema validator though just by looking at the error message and finding where/why it's reported. Finally after fixing all of those with the following diff: diff --git a/tests/qemuxml2argvdata/disk-network-nfs.xml b/tests/qemuxml2argvdata/disk-network-nfs.xml index 8ff6859f1f..67d2843e01 100644 --- a/tests/qemuxml2argvdata/disk-network-nfs.xml +++ b/tests/qemuxml2argvdata/disk-network-nfs.xml @@ -18,26 +18,26 @@ <driver name='qemu' type='raw' cache='none'/> <source protocol='nfs' name='/foo/bar/baz'> <host name='example.com' port='2049'/> - <nfs user='nfs-user' group='nfs-group'/> + <nfs user='+6234' group='+12354'/> </source> <target dev='vda' bus='virtio'/> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> - <disk> + <disk type='network' device='disk'> <driver name='qemu' type='qcow2'/> <source protocol='gluster' name='Volume2/Image'> <host transport='unix' socket='/path/to/sock'/> </source> <backingStore type='network' index='1'> - <format type='nfs'/> + <format type='qcow2'/> <source protocol='nfs' name='/backing/store/nfs'> <host name='example.org'/> - <nfs user='USER' group='GROUP'/> + <nfs user='+1234' group='+5678'/> </source> <backingStore/> </backingStore> - <target dev='vda' bus='virtio'/> + <target dev='vdb' bus='virtio'/> </disk> <controller type='usb' index='0'/> <controller type='pci' index='0' model='pci-root'/> I get a QMP schema validation error: 184) QEMU XML-2-ARGV disk-network-nfs.x86_64-latest ... failed to validate -blockdev '{"driver":"nfs","server":{"host":"example.com","port":"2049"},"path":"/foo/bar/baz","user":6234,"group":12354,"node-name":"libvirt-3-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}' against QAPI schema: { driver: 'nfs' OK server: { host: 'example.com': OK port: ERROR: attribute not in schema This means that the JSON definition does not conform to the QMP schema provided by qemu. Looking at the QMP schema, you used wrong generator for the 'server' portion: The QMP schema in qemu.git is written as: { 'struct': 'BlockdevOptionsNfs', 'data': { 'server': 'NFSServer', 'path': 'str', '*user': 'int', '*group': 'int', '*tcp-syn-count': 'int', '*readahead-size': 'int', '*page-cache-size': 'int', '*debug': 'int' } } { 'struct': 'NFSServer', 'data': { 'type': 'NFSTransport', 'host': 'str' } } { 'enum': 'NFSTransport', 'data': [ 'inet' ] } It seems that you've picked qemuBlockStorageSourceBuildJSONInetSocketAddress which formats 'SocketAddress'-compatible JSON, but the QMP schema for NFS has it's own type. Thus only TCP is supoprted and 'port' is not configurable. This needs to be expressed and checked in libvirt too so that users don't try to configure the port or unix transport while it's impossible. (This also shows why qemuxml2argvtest is so important, as it does the validation against qemu. I'm also fairly certain that qemu would reject such config, so please make sure to test your code before submitting it) > Also, both xml2argv and xml2xml tests are failing, even with the > VIR_REGENERATE_OUTPUT environment variable set to 1. The error is: > "libvirt: Domain Config error : missing source information for device vda" > To be completely honest, we don't know what about our XML or schema is > causing this error and so we aren't sure what the fix is. We've based our > XML mostly on disk-network-vxhs.xml and disk-backing-chains.xml, but don't > understand why those pass and ours doesn't. Because your XML is broken. If we can't parse it there's nothing to generate.
