[libvirt] [PATCH v2 0/2] Rearrange order in lxcDomainUpdateDeviceFlags
v1: https://www.redhat.com/archives/libvir-list/2018-June/msg01706.html Changes since v1, - Insert patch 1 to remove the FORCE flag from the possible/allowed list of flags - it was only useful for live anyway - Continue to rearrange things as suggested from review. John Ferlan (2): lxc: Remove FORCE flag from lxcDomainUpdateDeviceFlags lxc: Rearrange order in lxcDomainUpdateDeviceFlags src/lxc/lxc_driver.c | 66 ++++++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 44 deletions(-) -- 2.14.4
Force would be used to force eject a cdrom live, since the code doesn't support live update, remove the flag. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f9794e0655..aea48e5e3d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4832,8 +4832,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG | - VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); + VIR_DOMAIN_AFFECT_CONFIG, -1); if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; -- 2.14.4
Although commit e3497f3f noted that the LIVE option doesn't matter and removed the call to virDomainDefCompatibleDevice, it didn't go quite far enough and change the order of the checks and rework the code to just handle the config change causing a failure after virDomainObjUpdateModificationImpact updates the @flags. Since we only support config a lot of previously conditional code is now just inlined. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 63 ++++++++++++++++++---------------------------------- 1 file changed, 21 insertions(+), 42 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index aea48e5e3d..72c6daa507 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4827,7 +4827,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefPtr dev = NULL; int ret = -1; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); @@ -4846,61 +4846,40 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) goto endjob; - if (!(caps = virLXCDriverGetCapabilities(driver, false))) - goto endjob; - - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, - caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE); - if (dev == NULL) + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unable to modify live devices")); goto endjob; - - if (flags & VIR_DOMAIN_AFFECT_CONFIG && - flags & VIR_DOMAIN_AFFECT_LIVE) { - /* If we are affecting both CONFIG and LIVE - * create a deep copy of device as adding - * to CONFIG takes one instance. - */ - dev_copy = virDomainDeviceDefCopy(dev, vm->def, - caps, driver->xmlopt); - if (!dev_copy) - goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - /* Make a copy for updated domain. */ - vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); - if (!vmdef) - goto endjob; + if (!(caps = virLXCDriverGetCapabilities(driver, false))) + goto endjob; - /* virDomainDefCompatibleDevice call is delayed until we know the - * device we're going to update. */ - if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) - goto endjob; - } + if (!(dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, + VIR_DOMAIN_DEF_PARSE_INACTIVE))) + goto endjob; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("Unable to modify live devices")); + /* Make a copy for updated domain. */ + vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); + if (!vmdef) + goto endjob; + /* virDomainDefCompatibleDevice call is delayed until we know the + * device we're going to update. */ + if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) goto endjob; - } - /* Finally, if no error until here, we can save config. */ - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; } + endjob: virLXCDomainObjEndJob(driver, vm); cleanup: virDomainDefFree(vmdef); - if (dev != dev_copy) - virDomainDeviceDefFree(dev_copy); virDomainDeviceDefFree(dev); virDomainObjEndAPI(&vm); virObjectUnref(caps); -- 2.14.4
On 06/30/2018 04:51 PM, John Ferlan wrote:
Although commit e3497f3f noted that the LIVE option doesn't matter and removed the call to virDomainDefCompatibleDevice, it didn't go quite far enough and change the order of the checks and rework the code to just handle the config change causing a failure after virDomainObjUpdateModificationImpact updates the @flags. Since we only support config a lot of previously conditional code is now just inlined.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 63 ++++++++++++++++++---------------------------------- 1 file changed, 21 insertions(+), 42 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index aea48e5e3d..72c6daa507 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4827,7 +4827,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virCapsPtr caps = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; + virDomainDeviceDefPtr dev = NULL; int ret = -1; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
- }
+ ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef); + if (!ret) { + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; }
Well, this can be written a bit nicer: if (virDomainSaveConfig(cfg->configDir, driver->caps, vmdef) < 0) goto endjob; virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; ret = 0; Michal
On 06/30/2018 04:51 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-June/msg01706.html
Changes since v1,
- Insert patch 1 to remove the FORCE flag from the possible/allowed list of flags - it was only useful for live anyway
- Continue to rearrange things as suggested from review.
John Ferlan (2): lxc: Remove FORCE flag from lxcDomainUpdateDeviceFlags lxc: Rearrange order in lxcDomainUpdateDeviceFlags
src/lxc/lxc_driver.c | 66 ++++++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 44 deletions(-)
ACK Michal
participants (2)
-
John Ferlan -
Michal Prívozník