3:15 p.m.
On 02/09/2016 03:11 PM, Laine Stump wrote:
On 02/09/2016 10:59 AM, Cole Robinson wrote:
> We use the PreFormat callback for this. Many test cases need to be extended
> to pass in proper qemuCaps flags so AssignAddresses doesn't throw errors.
>
> One test case (pcie-root-port-too-many) is dropped, since it was meant
> only for checking an error condition in qemuxml2argv, and one we add in
> AssignAddresses it errors here too.
>
> Long term I think AssignAddresses should be handled in qemu's PostParse
> callback, but that's not entirely straightforward. Handling it here
> means we can get the test suite churn over with.
> ---
I'm too lazy to do it myself, but it would be comforting to move the
xml2xmlout data files into the xml2argvdata directory and run qemuxml2argvtest
with the existing .args files to verify that all of these PCI addresses you've
just generated are exactly the same as the ones that have been auto-generated
over the last several years and put into the .args files...
I gave it a spin:
cd tests/qemuxml2xmloutdata; for i in *.xml; do new=`echo $i | sed
"s/qemuxml2xmlout-/qemuxml2argv-/g"`; cp $i ../qemuxml2argvdata/$new; done
qemuxml2argvtest passes with no problems.
But qemuxml2xmltest actually fails now! Nothing major though, just the
controllers being reordered a bit in a few test cases. The qemu PostParse bits
should probably be adding controllers in the same order as domain_conf parses
them in, so the bits match. A patch for another day though
(Hmm - if the xml files in all the test data directories had their
"qemuxml2XXX-" prefixes removed as we had earlier discussed, such an operation
would be trivial :-)
Yup, it's still on my list.
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index e3b61c0..a06aa4d 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -37,13 +37,24 @@ struct testInfo {
> };
> static int
> +qemuXML2XMLPreFormatCallback(virDomainDefPtr def, const void *opaque)
> +{
> + const struct testInfo *info = opaque;
> +
> + if (qemuDomainAssignAddresses(def, info->qemuCaps, NULL))
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> testXML2XMLActive(const void *opaque)
> {
> const struct testInfo *info = opaque;
> return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
> info->inName, info->outActiveName,
> true,
> - NULL, NULL);
> + qemuXML2XMLPreFormatCallback, opaque);
> }
> @@ -54,7 +65,7 @@ testXML2XMLInactive(const void *opaque)
> return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt,
> info->inName,
> info->outInactiveName, false,
> - NULL, NULL);
> + qemuXML2XMLPreFormatCallback, opaque);
> }
> @@ -138,6 +149,9 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
> goto cleanup;
> }
> + if (qemuDomainAssignAddresses(obj->def, data->qemuCaps, NULL))
> + goto cleanup;
> +
Why is this needed? I though that's what the pre-format callback was for.
This function, for testing domain status XML, doesn't use the standard
testutils infrastructure because it has some funky XML handling, so this call
needs to be opencoded.
> /* format it back */
> if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL,
> VIR_DOMAIN_DEF_FORMAT_SECURE))) {
> @@ -379,14 +393,28 @@ mymain(void)
> DO_TEST("disk-drive-network-rbd-ipv6");
> DO_TEST("disk-drive-network-rbd-ceph-env");
> DO_TEST("disk-drive-network-sheepdog");
> - DO_TEST("disk-scsi-device");
> + DO_TEST_FULL("disk-scsi-device", WHEN_ACTIVE,
> + QEMU_CAPS_NODEFCONFIG,
> + QEMU_CAPS_SCSI_LSI);
The indentation of all of the followon lines to DO_TEST_FULL()'s are off. It's
consistent, but since it doesn't match what all of our editors likely do
automatically, so it will start to look ugly as new cases are added. (I didn't
look - is the indentation also incorrect in qemuxml2argvtest.c (which is where
I assume you got the caps lists for each test)?)
I didn't reflow anything in qemuxml2argtest I don't think. The nature of the
additions/removals means the indentation didn't change.
ACK with the question about the extra call to
qemuDomainAssignAddresses()
answered and the indentation fixed so that it matches what an editor's
auto-indent would do (putting the following lines one space past the column of
the opening "(" )
Honestly I'm not a big fan of column aligning, if there's any future
find+replace function rename the indentation is off... there's many examples
of this in libvirt code. But I fixed these cases manually and pushed now.
Thanks for the reviews!
- Cole