[libvirt] [PATCH v2 5/6] tests: qemuxml2xml: assign device addresses

[Cole Robinson]

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