[libvirt] [PATCH v2 3/7] tests: qemuxml2argv: Add infrastructure for testing with real qemuCaps

[Peter Krempa]

On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
> On Wed, Apr 18, 2018 at 11:38:43AM +0200, Peter Krempa wrote:
> > Allow testing of XML->argv conversion with using a real capability map
> > as used in the qemucapabilitiestest. This allows specifying the required
> > qemu version with the test rather than having to enumerate all the
> > required capabilities or allows to use the newest capabilities present.
> > 
> > Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> > ---
> > tests/qemuxml2argvtest.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> > 
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 31218652ce..75a9d0b908 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -593,6 +593,7 @@ mymain(void)
> >     int ret = 0;
> >     char *fakerootdir;
> >     bool skipLegacyCPUs = false;
> > +    char *capsnew_x86_64 = NULL;
> > 
> >     if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
> >         fprintf(stderr, "Out of memory\n");
> > @@ -658,6 +659,73 @@ mymain(void)
> >     if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, "/var/lib/libvirt/qemu/ram") < 0)
> >         return EXIT_FAILURE;
> > 
> > +    if (!(capsnew_x86_64 = testQemuGetNewestCapsForArch(abs_srcdir "/qemucapabilitiesdata",
> > +                                                        "x86_64", ".xml")))
> > +        return EXIT_FAILURE;
> > +
> > +    VIR_TEST_VERBOSE("\ncaps x86_64: %s\n", capsnew_x86_64);
> > +
> > +
> > +/**
> > + * The following set of macros allows testing of XML -> argv conversion with a
> > + * real set of capabilities gathered from a real qemu copy. It is desired to use
> > + * these for positive test cases as it provides combinations of flags which
> > + * can be met in real life.
> > + *
> > + * The capabilities are taken from the real capabilities stored in
> > + * tests/qemucapabilitiesdata.
> > + *
> > + * It is suggested to use the DO_TEST_CAPS_NEW macro which always takes the
> > + * newest capability set. In cases when the new capabilities would remove a
> > + * feature for some reason, the test should be forked by using DO_TEST_CAPS_VER
> > + * with the old version. New version of the capabilities can then be changed to
> > + * the new format.
> 
> The main point of these unit tests so far was to check whether changes
> and refactors of the libvirt code do not affect the .args output for old
> QEMUs.

Yes, but that's not the whole story.

> Even introducting new capabilities could cause libvirt to take different
> code paths, therefore we should prefer DO_TEST_CAPS_VER.

This is actually the exact reason why I think DO_TEST_CAPS_NEW is better
than DO_TEST_CAPS_VER. If we do changes to code used in multiple places
which are gated on presence of a capability, the places using
DO_TEST_CAPS_VER could change without us being able to catch that.

> I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
> new capability that needs to be handled by libvirt, the code author
> should introduce a new DO_TEST_CAPS_VER test for that.

In such reason, the author adding the code should fork the test by
adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
then add the new capability bit. With such approach it will be even
visible which options changed.

> Otherwise it would just be checking whether QEMU did not break something
> for us. And since the soonest we update capabilities is at the time of

Actually no. It will also check that something other will not break:

https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
tried to introduce a new property for the memory-backing-file object.
This object is used in multiple places. The property is gated by a
capability bit. Our tests were not able to catch, that this would add
the property to the NVDIMM device which needs to persist the data, which
would actually break it.

While I agree that DO_TEST_CAPS_VER is very useful for checking old
command line, I think that the true value is also in DO_TEST_CAPS_NEW
when used together with DO_TEST_CAPS_VER, so when a new addition would
change some tests using DO_TEST_CAPS_NEW we should fork them to cover
both recent and old versions.

> QEMU freeze, that might be later than needed.
> 
> 
> With the comment fixed to encourage DO_TEST_CAPS_VER:

I can reword it but I disagree that DO_TEST_CAPS_VER should be used
primarily.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180418/b9b914bb/attachment-0001.sig>