Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Tue, Dec 29, 2020 at 10:17:31 -0600, Ryan Gahagan wrote:
> On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa  wrote:
> 
> > Please don't post screenshots of text. It's pointlessly bloating the
> > mail and I can't respond inline. Please re-post all the relevant bits as
> > text.

I've remembered a few other reasons to avoid screenshots. Mostly I can't
copy if I wanted to look for something you've posted and also the
contents of the image are not indexed by search engines.

Also please note that the reviews may seem nitpicky, but I'm trying to
be thorough. By merging your code we'll need to support and maintain the
code in the future even in cases when you decide to no longer
participate in our project, thus we need to avoid as many future
problems as possible.

> Sorry about that, I didn't know what the policy on images was. Basically,
> we used the REGENERATE_OUTPUT for the xml2xmltest and inside the generated
> output there is a line which reads " index='1'>". However, the xml2xmltest fails on test 181 with the error:
> Expect [ index='1'>]
> Actual [>]
> 
> This backingStore is the only place in the file with an index='1', so
> logically we tried removing that attribute. However, this causes test 182
> to fail (even though it passed before this change) with the error:
> Expect [>]
> Actual [ index='1'>]
> 
> It seems like these tests are at odds with one another and we aren't sure
> what the cause or fix would be. If we use an index, then 181 fails, and if
> we don't, then 182 will fail. Do you know what might be causing this?

Ah, yes. That test is a bit magic when the definition XML of a live VM
differs from a definition XML of an inactive VM and in such case
VIR_TEST_REGENRATE_OUTPUT=1 will not do the right thing fully
automatically.

You need to create tests/qemuxml2xmloutdata/$TESTNAME-inactive.xml as an
empty file and then re-REGENERATE the output. The presence of the
-inactive file is registered by the test runner and then two distinct
versions are allowed.

Also note that if you provided full text output of the test I would be
able to replace $TESTNAME by the actual file name you need to use, by
copying and pasting.

Also, in this case it wasn't necessary, but always provide at least link
to your repository if you've modified the code. Making reviewers job as
easy as possible is the best way to expedite the review process. Review
is a rather scarse resource in many projects, so having the reviwer
guess or look for additional data wastes the available resource.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Tue, Dec 29, 2020 at 10:11 AM Peter Krempa  wrote:

> Please don't post screenshots of text. It's pointlessly bloating the
> mail and I can't respond inline. Please re-post all the relevant bits as
> text.
>

Sorry about that, I didn't know what the policy on images was. Basically,
we used the REGENERATE_OUTPUT for the xml2xmltest and inside the generated
output there is a line which reads "". However, the xml2xmltest fails on test 181 with the error:
Expect [ index='1'>]
Actual [>]

This backingStore is the only place in the file with an index='1', so
logically we tried removing that attribute. However, this causes test 182
to fail (even though it passed before this change) with the error:
Expect [>]
Actual [ index='1'>]

It seems like these tests are at odds with one another and we aren't sure
what the cause or fix would be. If we use an index, then 181 fails, and if
we don't, then 182 will fail. Do you know what might be causing this?

Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Mon, Dec 28, 2020 at 15:49:17 -0600, Ryan Gahagan wrote:
> Our output file (qemuxml2xmloutdata/disk-network-nfs.x86_64-latest.xml) has
> a backingStore on line 35. Test 181 generates an output where this backing
> store does not have an index, but Test 182 generates an output where this
> backing store has an index='1' attribute. Using REGENERATE_OUTPUT command
> seems to satisfy test 182 but fails test 181. If we fix the XML to satisfy
> test 181, then test 182 will fail. We've attached some screenshots for
> clarity, if this sounds confusing.

Please don't post screenshots of text. It's pointlessly bloating the
mail and I can't respond inline. Please re-post all the relevant bits as
text.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Wed, Dec 23, 2020 at 14:28:04 -0600, Ryan Gahagan wrote:
> On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa  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
> .
> 
> 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  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:


  
  

  
  


  
  


  
  


Firstly 'type' and 'device' are mandatory attributes for the 
element.

Also you've got two disks with  but the target must be
unique.

Also  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 @@
   
   
 
-
+
   
   
   eb90327c-8302-4725-9e1b-4e85ed4dc251
   
 
-
+
   
   
 
   
   
-
+
 
   
-  
+  
 
 
   
-  
+  
 
 
 

I get a QMP schema validation error:

184) 

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Wed, Dec 23, 2020 at 11:45 AM Peter Krempa  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?


> 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
.

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"

This seems like there's a problem with our XML not matching the RNG docs.
However, the problem appears to be with our  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?

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.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Mon, Dec 21, 2020 at 17:10:35 -0600, Ryan Gahagan wrote:
> We were able to get the testing environment running after some
> trial-and-error. The issue was that the version of libtirpc was out of
> date, causing an error in meson.build when the XDR dependency was being
> read. By updating the libtirpc-dev package we were able to start building
> locally. However, we did have some questions about our tests. For now we
> wanted to focus specifically on the qemublocktest.c file.
> 
> We have the function virDomainDiskSourceNetworkParse in
> src/conf/domain_conf.c where XML is parsed into virStorageSource data and
> the function qemuBlockStorageSourceCreateGetStorageProps under
> src/qemu/qemu_block.c where this data is converted into a JSON object. Our
> understanding of the tests/qemublocktest.c file is that we provide an XML
> string example via TEST_JSON_FORMAT_NET and that this string was parsed
> into JSON then formatted back into XML to make sure the process produced
> the same output as the input. One issue is that for some reason our user
> and group strings (i.e. “”) are not getting
> parsed into integers. The nfs_uid and nfs_gid fields are never set and
> therefore result in a 0 every time. We have code to fix this in a method
> under qemuDomainPrepareStorageSourceBlockdev in the file
> src/qemu/qemu_domain.c which looks up the user and group and stores the
> values we expect, but this method isn’t getting called. Where in this
> process have we missed something? Should we move the virGetUserID and
> virGetGroupID method calls to somewhere else?

No. The issue is that the tests shouldn't really be allowed to look up
the UID/GID in the system. For that reason the test code doesn't
actually call qemuDomainPrepareStorageSourceBlockdev. 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.

> We were also curious about the methods which parse the JSON values back
> into data. In our case, this is virStorageSourceParseBackingJSONNFS in
> src/util/virstoragefile.c. This method tries to retrieve the user and group
> integers from the NFS JSON object. However, when constructing the JSON
> object in the getStorageProps method we don't actually add those values if
> they're "default" values. In JSON terms this means that the integers are
> undefined, but how does C interpret it? Does the retrieve method return 0
> or simply fail?

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.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

We were able to get the testing environment running after some
trial-and-error. The issue was that the version of libtirpc was out of
date, causing an error in meson.build when the XDR dependency was being
read. By updating the libtirpc-dev package we were able to start building
locally. However, we did have some questions about our tests. For now we
wanted to focus specifically on the qemublocktest.c file.

We have the function virDomainDiskSourceNetworkParse in
src/conf/domain_conf.c where XML is parsed into virStorageSource data and
the function qemuBlockStorageSourceCreateGetStorageProps under
src/qemu/qemu_block.c where this data is converted into a JSON object. Our
understanding of the tests/qemublocktest.c file is that we provide an XML
string example via TEST_JSON_FORMAT_NET and that this string was parsed
into JSON then formatted back into XML to make sure the process produced
the same output as the input. One issue is that for some reason our user
and group strings (i.e. “”) are not getting
parsed into integers. The nfs_uid and nfs_gid fields are never set and
therefore result in a 0 every time. We have code to fix this in a method
under qemuDomainPrepareStorageSourceBlockdev in the file
src/qemu/qemu_domain.c which looks up the user and group and stores the
values we expect, but this method isn’t getting called. Where in this
process have we missed something? Should we move the virGetUserID and
virGetGroupID method calls to somewhere else?
We were also curious about the methods which parse the JSON values back
into data. In our case, this is virStorageSourceParseBackingJSONNFS in
src/util/virstoragefile.c. This method tries to retrieve the user and group
integers from the NFS JSON object. However, when constructing the JSON
object in the getStorageProps method we don't actually add those values if
they're "default" values. In JSON terms this means that the integers are
undefined, but how does C interpret it? Does the retrieve method return 0
or simply fail?

Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Fri, Dec 18, 2020 at 15:14:01 -0600, Ryan Gahagan wrote:
> On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa  wrote:
> 
> > Also note that the upstream test-suite run in the CI does actually
> > provide the expected output. Obviously you can't use the ENV variable to
> > automatically overwrite your files, but you certainly can copy out the
> > diffs from the CI. Just commit empty files in place for the output files
> > and the CI will complain that they differ including the full difference.
> >
> 
> We tried doing this and we failed on CI, but the pipeline jobs didn't
> output a diff, and instead simply errored out with the message "Full log
> written to
> /builds/bschoney/libvirt/build/meson-private/dist-build/meson-logs/testlog.txt".
> How can we actually view the diff against our blank file? We're trying to
> find the output for the qemuxml2argvdata args file for our new test but
> it's not being printed anywhere.


You can actually see some of the test output in your CI pipeline runs:

https://gitlab.com/bschoney/libvirt/-/jobs/922060289#L2961

anyways, it's not worth for me to debug why some of the output might be
skipped (perhaps missing empty output files).

Please make sure your local env is able to build libvirt, so that you
can use VIR_TEST_REGENERATE_OUTPUT. That is certainly the simplest way.
In addition you certainly want to test your changes in action, and our
CI AFAIK doesn't provide built packages.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

On Thu, Dec 17, 2020 at 8:00 AM Peter Krempa  wrote:

> Also note that the upstream test-suite run in the CI does actually
> provide the expected output. Obviously you can't use the ENV variable to
> automatically overwrite your files, but you certainly can copy out the
> diffs from the CI. Just commit empty files in place for the output files
> and the CI will complain that they differ including the full difference.
>

We tried doing this and we failed on CI, but the pipeline jobs didn't
output a diff, and instead simply errored out with the message "Full log
written to
/builds/bschoney/libvirt/build/meson-private/dist-build/meson-logs/testlog.txt".
How can we actually view the diff against our blank file? We're trying to
find the output for the qemuxml2argvdata args file for our new test but
it's not being printed anywhere.

Re: RFC PATCH: Issue 90 (Test Clarification)

[Peter Krempa]

On Wed, Dec 16, 2020 at 19:37:48 -0600, Ryan Gahagan wrote:
> We addressed the feedback from our previous RFC patch for the most part.
> Under src/util/virstoragefile.c, we left a cast to an integer pointer
> that Peter mentioned because we were unable to provide a better

Casting uid_t * to int * will work in this instance but is not
acceptable. The problem with casting pointers is that if sizeof(uid_t)
!= sizeof(int) casting a pointer could result into accessing invalid
memory, while if you just assign the value of integers of distinct sizes
you get an overflow at worst. That must be changed in the code.

> solution. We've written some tests for our code but our testing
> environment is not working locally (meson doesn't even recognize the
> project as a meson build project) and so we can't regenerate output or
> test our tests.

Could you elaborate? As in, post exact steps and output what you've done
and what's broken.

> It's probably bad practice but the only solution we could think of that
> would allow us to check our tests was just to email you what we've got.
> Sorry if it's not up to standard, but please let us know if there's a
> better way to do it or if you spot any problems in these tests.

It's better to commit what you have and ask directly, ideally including
output you are (not) getting.

> Under tests/qemuxml2xmltest.c:
> DO_TEST_CAPS_LATEST("disk-network-nfs");
> 
> The same line would be in tests/qemuxml2argvtest.c
> 
> We created the file tests/qemuxml2argvdata/disk-network-nfs.xml:
> 
>   QEMUGuest1
>   c7a5fdbd-edaf-9455-926a-d65c16db1809
>   219136
>   219136
>   1
>   
> hvm
> 
>   
>   
>   destroy
>   restart
>   destroy
>   
> /usr/bin/qemu-system-x86_64
> 
>   
>   
> 
> 
>   
>   
>   eb90327c-8302-4725-9e1b-4e85ed4dc251
>function='0x0'/>
> 
> 
> 
> 
> 
> 
>   
> 
> 
> We have under tests/qemublocktest.c:
> TEST_JSON_FORMAT_NET(“\n”
>  “  \n”
>  “  \n”
>  "\n”);
> and
> TEST_IMAGE_CREATE(“network-nfs-qcow2”, NULL);
> 
> And finally under tests/virstoragetest.c:
> TEST_BACKING_PARSE(“json:{\”file\”:{\”driver\”:\”nfs\”,”
>“\”user\”:\”USER\”,”
>“\”group\”:\”GROUP\”,”
>“\”server\”: {  \”host\”:\”example.com\”,”
>   “\”port\”:\”2049\””
>”}”
>   “}”
> “}”,
>“\n”
>“  \n”
>“  \n”
>“\n”);
> 
> Again, sorry if this looks awful. Please let us know if there's a more
> practical way to do this because submitting a commit with these tests
> would guarantee that the tests fail and the commit wouldn't be mergeable
> due to our environment issues, or if you see anything wrong with these
> tests.

Note that patches without tests are not acceptable either. Additionally
I'll not be copying your changes to appropriate places. Please add the
test code to appropriate places.

Also note that the upstream test-suite run in the CI does actually
provide the expected output. Obviously you can't use the ENV variable to
automatically overwrite your files, but you certainly can copy out the
diffs from the CI. Just commit empty files in place for the output files
and the CI will complain that they differ including the full difference.

If you don't post what's wrong I will not be guessing, so if you want
any input from us, be sure to provide as much information as you have
including exact steps.

RFC PATCH: Issue 90 (Test Clarification)

[Ryan Gahagan]

We addressed the feedback from our previous RFC patch for the most part.
Under src/util/virstoragefile.c, we left a cast to an integer pointer
that Peter mentioned because we were unable to provide a better
solution. We've written some tests for our code but our testing
environment is not working locally (meson doesn't even recognize the
project as a meson build project) and so we can't regenerate output or
test our tests.

It's probably bad practice but the only solution we could think of that
would allow us to check our tests was just to email you what we've got.
Sorry if it's not up to standard, but please let us know if there's a
better way to do it or if you spot any problems in these tests.

Under tests/qemuxml2xmltest.c:
DO_TEST_CAPS_LATEST("disk-network-nfs");

The same line would be in tests/qemuxml2argvtest.c

We created the file tests/qemuxml2argvdata/disk-network-nfs.xml:

  QEMUGuest1
  c7a5fdbd-edaf-9455-926a-d65c16db1809
  219136
  219136
  1
  
hvm

  
  
  destroy
  restart
  destroy
  
/usr/bin/qemu-system-x86_64

  
  


  
  
  eb90327c-8302-4725-9e1b-4e85ed4dc251
  






  


We have under tests/qemublocktest.c:
TEST_JSON_FORMAT_NET(“\n”
 “  \n”
 “  \n”
 "\n”);
and
TEST_IMAGE_CREATE(“network-nfs-qcow2”, NULL);

And finally under tests/virstoragetest.c:
TEST_BACKING_PARSE(“json:{\”file\”:{\”driver\”:\”nfs\”,”
   “\”user\”:\”USER\”,”
   “\”group\”:\”GROUP\”,”
   “\”server\”: {  \”host\”:\”example.com\”,”
  “\”port\”:\”2049\””
   ”}”
  “}”
“}”,
   “\n”
   “  \n”
   “  \n”
   “\n”);

Again, sorry if this looks awful. Please let us know if there's a more
practical way to do this because submitting a commit with these tests
would guarantee that the tests fail and the commit wouldn't be mergeable
due to our environment issues, or if you see anything wrong with these
tests.