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

[Andrea Bolognani]

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.

> +                                     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().

> @@ -153,9 +149,6 @@ testCompareStatusXMLToXMLFiles(const void *opaque)
>          goto cleanup;
>      }
>  
> -    if (qemuDomainAssignAddresses(obj->def, data->qemuCaps, NULL))
> -        goto cleanup;
> -
>      /* format it back */
>      if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL,
>                                        VIR_DOMAIN_DEF_FORMAT_SECURE))) {

ACK

-- 
Andrea Bolognani
Software Engineer - Virtualization Team