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