[PATCH] libxl: Normalize MAC address in device conf when hotplugging a netdev
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. Signed-off-by: Jim Fehlig <jfehlig@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, + 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; -- 2.26.0
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@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@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;
On 6/2/20 9:58 PM, Laine Stump wrote:
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" :-)
Agreed, I'll add it. Thanks for the details.
Signed-off-by: Jim Fehlig <jfehlig@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? :-)
It made me think of milk :-).
(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!))
Haha, that explains it! I think I would have preferred a dairy farm over the steel mills and refineries of my childhood :-).
At any rate, it all looks good
Reviewed-by: Laine Stump <laine@redhat.com>
Thanks for the review! Regards, Jim
participants (2)
-
Jim Fehlig -
Laine Stump