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