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