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@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
+ * config versions of DeviceDef, but might not be. Do this by
+ * changing the contents of devLive.
+ */
+
+ /* 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()
+ */
+ 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;
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 12/17/19 1:45 PM, Michal Prívozník wrote:
> On 12/17/19 6:04 PM, Laine Stump wrote:
...
>> +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:
Yeah, I thought of adding a warning, and actually was going to
specifically mention that PCI/etc addresses shouldn't be changed (since
that was something people have already talked about adjusting), but then
forgot. I'll add in something like you suggest before pushing.
Thanks!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.