[libvirt] [PATCH 2/4] qemu: Assign device addresses in PostParse

[Cole Robinson]

On 05/17/2016 10:39 AM, Andrea Bolognani wrote:
> On Sat, 2016-05-14 at 16:00 -0400, Cole Robinson wrote:
>> This wires up qemuDomainAssignAddresses into the new
>> virDomainDefAssignAddressesCallback, so it's always triggered
>> via virDomainDefPostParse. We are essentially doing this already
>> with open coded calls sprinkled about
> 
> Missing period.
> 
>> qemu argv parse output changes slightly since previously it wasn't
>> hitting qemuDomainAssignAddresses.
>> ---
>>   src/qemu/qemu_domain.c                             | 25 ++++++++++++++++++++++
>>   .../qemuargv2xmldata/qemuargv2xml-pseries-disk.xml |  4 +++-
>>   tests/qemuxml2argvtest.c                           |  2 +-
>>   tests/qemuxml2xmltest.c                            | 11 ++--------
>>   4 files changed, 31 insertions(+), 11 deletions(-)
>>  
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index b0eb3b6..50505a6 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -2293,9 +2293,34 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
>>   }
>>   
>>   
>> +static int
>> +qemuDomainDefAssignAddressesCallback(virDomainDef *def,
>> +                                     virCapsPtr caps ATTRIBUTE_UNUSED,
>> +                                     unsigned int parseFlags ATTRIBUTE_UNUSED,
> 
> So these two arguments are unused, and they remain unused by
> the end of the series... I guess you wanted to have the same
> signature as virDomainDefPostParseCallback()?
> 
> Not sure if it's worth keeping them around, but I guess it's
> not a big deal either way.
> 

I was just doing it to be consistent with the other callbacks. I just left it
as is since I can imagine some day we are going to want to abide parseFlags at
least.

>> +                                     void *opaque)
>> +{
>> +    virQEMUDriverPtr driver = opaque;
>> +    virQEMUCapsPtr qemuCaps = NULL;
>> +    int ret = -1;
>> +
>> +    if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
>> +                                            def->emulator)))
>> +        goto cleanup;
>> +
>> +    if (qemuDomainAssignAddresses(def, qemuCaps, NULL) < 0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> + cleanup:
>> +    virObjectUnref(qemuCaps);
>> +    return ret;
>> +}
>> +
>> +
>>   virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
>>       .devicesPostParseCallback = qemuDomainDeviceDefPostParse,
>>       .domainPostParseCallback = qemuDomainDefPostParse,
>> +    .assignAddressesCallback = qemuDomainDefAssignAddressesCallback,
> 
> I'd say call it qemuDomainDefAssignAddresses for consistency's
> sake.
> 
>>       .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG |
>>                   VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN
>>   };
>> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml b/tests/qemuargv2xmldata/qemuargv2xml-pseries-
>> disk.xml
>> index 8cec27c..ab9195a 100644
>> --- a/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
>> +++ b/tests/qemuargv2xmldata/qemuargv2xml-pseries-disk.xml
>> @@ -29,7 +29,9 @@
>>       </disk>
>>       <controller type='usb' index='0'/>
>>       <controller type='pci' index='0' model='pci-root'/>
>> -    <controller type='scsi' index='0'/>
>> +    <controller type='scsi' index='0'>
>> +      <address type='spapr-vio' reg='0x2000'/>
>> +    </controller>
>>       <input type='keyboard' bus='usb'/>
>>       <input type='mouse' bus='usb'/>
>>       <graphics type='sdl'/>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index d1cfbec..840efc9 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -1382,7 +1382,7 @@ mymain(void)
>>               QEMU_CAPS_PCI_OHCI, QEMU_CAPS_PCI_MULTIFUNCTION);
>>       DO_TEST("pseries-vio-user-assigned",
>>               QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
>> -    DO_TEST_FAILURE("pseries-vio-address-clash",
>> +    DO_TEST_PARSE_ERROR("pseries-vio-address-clash",
>>               QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
>>       DO_TEST("pseries-nvram", QEMU_CAPS_DEVICE_NVRAM);
>>       DO_TEST("pseries-usb-kbd", QEMU_CAPS_PCI_OHCI,
>> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
>> index 5a43fa9..9eb2625 100644
>> --- a/tests/qemuxml2xmltest.c
>> +++ b/tests/qemuxml2xmltest.c
>> @@ -37,13 +37,9 @@ struct testInfo {
>>   };
>>   
>>   static int
>> -qemuXML2XMLPreFormatCallback(virDomainDefPtr def, const void *opaque)
>> +qemuXML2XMLPreFormatCallback(virDomainDefPtr def ATTRIBUTE_UNUSED,
>> +                             const void *opaque ATTRIBUTE_UNUSED)
>>   {
>> -    const struct testInfo *info = opaque;
>> -
>> -    if (qemuDomainAssignAddresses(def, info->qemuCaps, NULL))
>> -        return -1;
>> -
>>       return 0;
>>   }
> 
> If you're going to remove the whole function body, why not go
> the extra mile? Get rid of the function altogether and just
> pass NULL to testCompareDomXML2XMLFiles().
> 

This callback could be used for other types of testing, like assigning aliases
which we don't presently test. So I decided to leave it in.

Thanks,
Cole