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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list