[PATCH] libxl: Normalize MAC address in device conf when hotplugging a netdev

Laine Stump laine at redhat.com
Wed Jun 3 03:58:06 UTC 2020


On 5/28/20 1:09 PM, Jim Fehlig wrote:
> Similar to commit 6c17606b7cc, normalize the MAC addresses in persistent
> and live device config to avoid a different MAC address for the device
> once the VM is rebooted and persistent config takes affect.


Well...


This has a bigger change than commit 6c17606b7cc! :-)


For those who don't feel like digging back through the git blame 
history, commit 6c17606b7cc just added a call to 
qemuDomainAttachDeviceLiveAndConfigHomogenize() in the QEMU driver 
version of AttachDeviceFlags() to make the MAC addresses of the 
*already-existing* "live" and "config" copies of the device object match 
each other - qemu's function had previously been changed back in commit 
55ce6564 to create separate copies of the device object for config and 
live (although it still had to save an extra pointer to the config copy, 
which was being consumed by the the qemuDomainAttachDeviceConfig(), with 
the pointer NULLed


The libxl driver hadn't gotten that change though, so up until now it's 
only had a single device object pointer, and although it separately 
parsed the XML for live and config, this was done sequentially into the 
same object, so the two never existed at the same time. Since both 
objects must exist at the same time to copy anything from one to the 
other (without "extra steps"), this new patch is effectively the libxl 
equivalent of commit 55ce6564 and commit 6c17606b7cc put together.

That was a long prelude to "maybe mention commit 55ce6564 in the commit 
log as well" :-)


>
> Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> ---
>   src/libxl/libxl_driver.c | 56 +++++++++++++++++++++++++++++++++-------
>   1 file changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 63ec0a2188..a80bc3fe3a 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4096,6 +4096,31 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
>   }
>   
>   
> +static void
> +libxlDomainAttachDeviceNormalize(const virDomainDeviceDef *devConf,


What? You didn't like my verb in the QEMU version? :-)


(I can't remember why I picked Homogenize instead of Normalize, but I 
must have had some reason. Maybe I was just thinking back to the dairy 
farm next door to my childhood home (true story!))


At any rate, it all looks good


Reviewed-by: Laine Stump <laine at redhat.com>


> +                                 virDomainDeviceDefPtr devLive)
> +{
> +    /*
> +     * Fixup anything that needs to be identical in the live and
> +     * config versions of DeviceDef, but might not be. Do this by
> +     * changing the contents of devLive. This is done after all
> +     * post-parse tweaks and validation, so be very careful about what
> +     * changes are made.
> +     */
> +
> +    /* MAC address should be identical in both DeviceDefs, but if it
> +     * wasn't specified in the XML, and was instead autogenerated, it
> +     * will be different for the two since they are each the result of
> +     * a separate parser call. If it *was* specified, it will already
> +     * be the same, so copying does no harm.
> +     */
> +
> +    if (devConf->type == VIR_DOMAIN_DEVICE_NET)
> +        virMacAddrSet(&devLive->data.net->mac, &devConf->data.net->mac);
> +
> +}
> +
> +
>   static int
>   libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>                                unsigned int flags)
> @@ -4104,7 +4129,9 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>       libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>       virDomainObjPtr vm = NULL;
>       virDomainDefPtr vmdef = NULL;
> -    virDomainDeviceDefPtr dev = NULL;
> +    virDomainDeviceDefPtr devConf = NULL;
> +    virDomainDeviceDef devConfSave = { 0 };
> +    virDomainDeviceDefPtr devLive = NULL;
>       int ret = -1;
>   
>       virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_LIVE |
> @@ -4123,28 +4150,36 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>           goto endjob;
>   
>       if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) {
> -        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
> -                                            driver->xmlopt, NULL,
> -                                            VIR_DOMAIN_DEF_PARSE_INACTIVE)))
> +        if (!(devConf = virDomainDeviceDefParse(xml, vm->def,
> +                                                driver->xmlopt, NULL,
> +                                                VIR_DOMAIN_DEF_PARSE_INACTIVE)))
>               goto endjob;
>   
>           /* Make a copy for updated domain. */
>           if (!(vmdef = virDomainObjCopyPersistentDef(vm, driver->xmlopt, NULL)))
>               goto endjob;
>   
> -        if (libxlDomainAttachDeviceConfig(vmdef, dev) < 0)
> +        /*
> +         * devConf will be NULLed out by
> +         * libxlDomainAttachDeviceConfig(), so save it for later use by
> +         * libxlDomainAttachDeviceNormalize()
> +         */
> +        devConfSave = *devConf;
> +
> +        if (libxlDomainAttachDeviceConfig(vmdef, devConf) < 0)
>               goto endjob;
>       }
>   
>       if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
> -        /* If dev exists it was created to modify the domain config. Free it. */
> -        virDomainDeviceDefFree(dev);
> -        if (!(dev = virDomainDeviceDefParse(xml, vm->def,
> +        if (!(devLive = virDomainDeviceDefParse(xml, vm->def,
>                                               driver->xmlopt, NULL,
>                                               VIR_DOMAIN_DEF_PARSE_INACTIVE)))
>               goto endjob;
>   
> -        if (libxlDomainAttachDeviceLive(driver, vm, dev) < 0)
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> +            libxlDomainAttachDeviceNormalize(&devConfSave, devLive);
> +
> +        if (libxlDomainAttachDeviceLive(driver, vm, devLive) < 0)
>               goto endjob;
>   
>           /*
> @@ -4171,7 +4206,8 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
>   
>    cleanup:
>       virDomainDefFree(vmdef);
> -    virDomainDeviceDefFree(dev);
> +    virDomainDeviceDefFree(devConf);
> +    virDomainDeviceDefFree(devLive);
>       virDomainObjEndAPI(&vm);
>       virObjectUnref(cfg);
>       return ret;





More information about the libvir-list mailing list