On Mon, Oct 05, 2020 at 17:40:48 -0400, Nico Pache wrote:

I'm not super familiar with the code base so followed the 2 autodeflate commits as a guide.

I had a few questions regarding your feedback.

...

The patch is also missing test XML addition for the qemuxml2argvtest and

qemuxml2xmltest.

can you please explain the qemuxml2xmltest files. I don't get what they do.

qemuxml2argvtest takes the input file from qemuxml2argvdata/testname.xml which is a VM xml file and invokes internals which generate the command line which would be used to start qemu. This is then checked against qemuxml2argvdata/testname.args Similarly qemuxml2xmltest takes the same input file as the above test (literally the same, from the qemuxml2argv directory) and parses it and formats it back applying any internal validation and default filling. The output is compared against qemuxml2xmloutdata/testname.xml Note that you just have to provide the input file and invocation macro and you can then run the testsuite instructing it to generate the expected output files: VIR_TEST_REGENERATE_OUTPUT=1 tests/qemuxml2argvtest And commit the result after verifying that it has the new bits formatted. (Yes the testsuite will fail on the first round.) You can either provide your own input file, but it needs have certain settings (such as path to emulator) right to work with the tests so it's usually simpler to just copy and modify an appropriate existing test input file. The added bonus of the above is that those files are also automatically checked for conformance with the XML schema. Please note that any new tests added to the qemuxml2* test suite should use the DO_TEST_CAPS_* (such as DO_TEST_CAPS_LATEST, which invokes the test with capabilities of the latest version of x86_64 qemu) macros to invoke it rather than the now obsolete DO_TEST (without CAPS) where you are enumerating the qemu capability flags manually.

Also do I commit the additional testing files with the src/qemu changes or in a separate commit?

Either of them is fine. They just must apply, compile and pass the tests after every commit.

Will changing *virtio-balloon-pci.free-page-reporting* to *virtio-balloon.free-page-reporting* encompass all the possible virtio-balloon options?

yup, that sounds reasonable

Thanks in advance! --Nico