Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- src/conf/domain_conf.c | 12 +++--- src/libxl/libxl_driver.c | 97 ++++-------------------------------------------- src/lxc/lxc_driver.c | 75 +++---------------------------------- 3 files changed, 19 insertions(+), 165 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a9706b0..e54c097 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2880,13 +2880,11 @@ virDomainObjUpdateModificationImpact(virDomainObjPtr vm, return -1; } - if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("transient domains do not have any " - "persistent config")); - return -1; - } + if (!vm->persistent && (*flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domains do not have any " + "persistent config")); + return -1; } return 0; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d4e9c2a7..508bae4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1440,7 +1440,6 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - bool isActive; int ret = -1; virCheckFlags(VIR_DOMAIN_MEM_LIVE | @@ -1456,38 +1455,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_MEM_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE; - else - flags = VIR_DOMAIN_MEM_CONFIG; - } - if (flags == VIR_DOMAIN_MEM_MAXIMUM) { - if (isActive) - flags = VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_MAXIMUM; - else - flags = VIR_DOMAIN_MEM_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; - } - - if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot set memory on an inactive domain")); + if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, + &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_MEM_CONFIG) { - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, - driver->xmlopt, - vm))) - goto endjob; - } if (flags & VIR_DOMAIN_MEM_MAXIMUM) { /* resize the maximum memory */ @@ -3680,25 +3650,8 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify device on transient domain")); - goto endjob; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3788,25 +3741,8 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto endjob; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify device on transient domain")); - goto endjob; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, @@ -3893,25 +3829,8 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; - } else { - if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) - flags |= VIR_DOMAIN_DEVICE_MODIFY_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("Domain is not running")); - goto cleanup; - } - } - - if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto endjob; if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(xml, vm->def, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 24b9622..aea39c1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4989,43 +4989,22 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot do live update a device on " - "inactive domain")); - goto cleanup; - } - } - if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; dev = dev_copy = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt, @@ -5117,41 +5096,20 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainUpdateDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot do live update a device on " - "inactive domain")); - goto cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; @@ -5231,40 +5189,19 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, virDomainDefPtr vmdef = NULL; virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; int ret = -1; - unsigned int affect; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - affect = flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG); - if (!(vm = lxcDomObjFromDomain(dom))) goto cleanup; if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - if (affect == VIR_DOMAIN_AFFECT_CURRENT) - flags |= VIR_DOMAIN_AFFECT_CONFIG; - /* check consistency between flags and the vm state */ - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot do live update a device on " - "inactive domain")); - goto cleanup; - } - } - - if ((flags & VIR_DOMAIN_AFFECT_CONFIG) && !vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot modify device on transient domain")); - goto cleanup; - } + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + goto cleanup; if (!(caps = virLXCDriverGetCapabilities(driver, false))) goto cleanup; -- 1.8.3.1

On 02.02.2016 01:48, John Ferlan wrote: Well it is not principal to me. I thought as new and old logically equivalent we can get rid of extra nesting. (By the way we can exchage operands of && as they don't influence each other.) that place needs extra explanations, see below This place is strange but correct at least until no extra memory flags introduced. Basically these two if blocks resolve 'current' flag but instead of checking flags against 'live & config' mask as in virDomainObjUpdateModificationImpact the resolving is expanded into two blocks.

On 19.02.2016 01:43, John Ferlan wrote: Thank you for pointing this out. Somehow I failed to realize it and started to appreciate this rule only recently ) Ok. Sure, this change deserves its own commit with all explanations in commit message.

On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote: This one seems reasonable and true. John

On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote: You've based your assumptions off of the change you made in 1/3 to virDomainObjUpdateModificationImpact regarding combining that if statement. Also even if the following would fail, you would have need to provide some sort of virReportError. John

ping On 02.02.2016 16:18, Nikolay Shirokovskiy wrote:

On 19.02.2016 02:19, John Ferlan wrote: Yes, it quite mangled place. I think we'd better stick with next definitions as this is how we seem to use them in other places. 1. transient config - config for a running domain, lost after it is destroyed. 2. persistent config - config on disk for a domain. Looking at the code I've found next correnspondence of def and newDef to this definitions: 1. active persistent domain def - transient newDef - persistent 2. inactive persistent domain def - persistent 3. transient domain (active) def - transient Thus newDef has only one purpose - persistent config of active persistent domain. def could be transient or persistent depends on situation. Now one can understand the reason for the function virDomainObjSetDefTransient naming. On persistent domain start we move from 'persistent' to 'transient' for 'def' meaining. 'live' flags helps us do it before actual start is happend. I think such a function would be hard to use as usually we need to distinguish transient/persistent and have no notion of 'active'. Hmm ... if someone knows it is transient domain or inactive persistent domain it can refer to config without specifying it further as transient or persistent. Well in this case he can just use plain 'def' instead of this function. Another common usage of def is the case we need 'transient' config. It is always in 'def'. This is why we don't have virDomainObjGetTransientDef. Yes that check is there because there is no need for newDef for a transient domain. But I think the reason virDomainObjGetPersistentDef calls virDomainObjSetDefTransient is different. 'Set' function is kind of lazy initialization tool. We initialize newDef only when someone wants change persistent config for a running persistent domain for the first time. This is the reason this check is there: if(domain->newDef) return 0; However I doubt we need this lazy initialization there anymore. After cb4c2694f166beca8f3e1fb37dedeec1ff3fbdf6 commit newDef is initialized early in the process of starting of domain for lxc and qemu drivers at least. It will take time to investigate it... Just get here. virDomainObjGetDefs has the exact description of correspondence I wrote above in one place. Here it is 'live' config instead of 'transient'. Yes, if someone needs 'def' for transient domain - it needs 'transient' config and can use this function. Hmm, one asks why bother to mention 'transient' on config for transient domain as there is no other one. Well looks like we have to as we have one type for all three states - active persistent, active transient, inactive persistent. As to reporting error upon return NULL from the function. I think it is not right as if someone use the return value in this circumstances - it is a case of incorrect code and moreover it will crash instanly. After all this investigation I would like to see one day all usages of def and newDef would be replaced by getTransient and getPersistent calls )) with all the mechanics incapsulated. Nikolay.