[libvirt] [PATCH] qemu: homogenize MAC address in live & config when hotplugging a netdev

[Michal Prívozník]

On 12/17/19 6:04 PM, Laine Stump wrote:
> Prior to commit 55ce6564634 (first in libvirt 4.6.0), the XML sent to
> virDomainAttachDeviceFlags() was parsed only once, and the results of
> that parse were inserted into both the live object of the running
> domain and into the persistent config. Thus, if MAC address was
> omitted from in XML for a network device (<interface>), both the live
> and config object would have the same MAC address.
> 
> Commit 55ce6564634 changed the code to parse the incoming XML twice -
> once for live and once for config. This does eliminate the problem of
> PCI (/scsi/sata) address conflicts caused by allocating an address
> based on existing devices in live object, but then inserting the
> result into the config (which may already have a device using that
> address), BUT it also means that when the MAC address of a network
> device hasn't been specified in the XML, each copy will get a
> different auto-generated MAC address.
> 
> This results in the MAC address of the device changing the next time
> the domain is shutdown and restarted, which creates havoc with the
> guest OS's network config.
> 
> There have been several discussions about this in the last > 1 year,
> attempting to find the ideal solution to this problem that makes MAC
> addresses consistent and accounts for all sorts of corner cases with
> PCI/scsi/sata addresses. All of these discussions fizzled out because
> every proposal was either too difficult to implement or failed to fix
> some esoteric case someone thought up.
> 
> So, in the interest of solving the MAC address problem while not
> making the "other address" situation any worse than before, this patch
> simply adds a qemuDomainAttachDeviceLiveAndConfigHomogenize() function
> that (for now) copies the MAC address from the config object to the
> live object (if the original xml had <mac address='blah'/> then this
> will be an effective NOP (as the macs already match)).
> 
> Any downstream libvirt containing upstream commit
> 55ce6564634 should have this patch as well.
> 
> https://bugzilla.redhat.com/1783411
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 924f01d3eb..19ddff80b5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8623,6 +8623,30 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
>      return 0;
>  }
>  
> +
> +static void
> +qemuDomainAttachDeviceLiveAndConfigHomogenize(const virDomainDeviceDef *devConf,
> +                                              virDomainDeviceDefPtr devLive)
> +{
> +    /*
> +     * fixup anything that needs to be identical in the live and

s/fixup/Fixup/

> +     * config versions of DeviceDef, but might not be. Do this by
> +     * changing the contents of devLive.

We need to warn everybody that only "small" changes are okay. I mean, we
probably don't have to explicitly warn about not changing PCI address,
but something among these lines should do:

Remember, this is done after post parse and all other checks, be very
careful!

> +     */
> +
> +    /* 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
>  qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>                                      virQEMUDriverPtr driver,
> @@ -8633,6 +8657,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>      virDomainDefPtr vmdef = NULL;
>      g_autoptr(virQEMUDriverConfig) cfg = NULL;
>      virDomainDeviceDefPtr devConf = NULL;
> +    virDomainDeviceDef devConfSave = { 0 };
>      virDomainDeviceDefPtr devLive = NULL;
>      int ret = -1;
>      unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> @@ -8657,6 +8682,13 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>                                                  parse_flags)))
>              goto cleanup;
>  
> +        /*
> +         * devConf will be NULLed out by
> +         * qemuDomainAttachDeviceConfig(), so save it for later use by
> +         * qemuDomainDeviceLiveAndConfigHomogenize()

This function was renamed ;-)

> +         */
> +        devConfSave = *devConf;
> +
>          if (virDomainDeviceValidateAliasForHotplug(vm, devConf,
>                                                     VIR_DOMAIN_AFFECT_CONFIG) < 0)
>              goto cleanup;
> @@ -8678,6 +8710,9 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>                                                  parse_flags)))
>              goto cleanup;
>  
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> +            qemuDomainAttachDeviceLiveAndConfigHomogenize(&devConfSave, devLive);
> +
>          if (virDomainDeviceValidateAliasForHotplug(vm, devLive,
>                                                     VIR_DOMAIN_AFFECT_LIVE) < 0)
>              goto cleanup;
> 

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

and consider squashing the following in:

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 19ddff80b5..0d4e9c69c2 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -8629,9 +8629,10 @@
qemuDomainAttachDeviceLiveAndConfigHomogenize(const virDomainDeviceDef
*devConf,
                                               virDomainDeviceDefPtr
devLive)
 {
     /*
-     * fixup anything that needs to be identical in the live and
+     * 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.
+     * changing the contents of devLive. Remember, this is done
+     * after post parse and all other checks, be very careful!
      */

     /* MAC address should be identical in both DeviceDefs, but if it
@@ -8685,7 +8686,7 @@
qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
         /*
          * devConf will be NULLed out by
          * qemuDomainAttachDeviceConfig(), so save it for later use by
-         * qemuDomainDeviceLiveAndConfigHomogenize()
+         * qemuDomainAttachDeviceLiveAndConfigHomogenize()
          */
         devConfSave = *devConf;


Michal