On 02.02.2016 01:48, John Ferlan wrote:
On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
Uses virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact appropriately.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- 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)) {
Not the same check.
A 'transient' domain is running, but has no on disk config.
So if some command (e.g. virsh $dom setmem 20G --config) is issued, we want to stop that from happening on a transient domain. However, there may be other commands executed on a transient domain that we want to allow to happen, thus we cannot change this into an && check. It needs to be "if attempting to affect config", then if not persistent, then error [else allow the change to the config].
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.)
The rest is libxl specific and while it seems reasonable, I didn't check each change... I did note there is at least one change which has command specific logic dealing with flags adjustments not related to active, live, persistent, config, current, etc. removed which doesn't seem like it's right...
that place needs extra explanations, see below
John
+ 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; - }
VIR_DOMAIN_MEM_MAXIMUM has nothing to do with CONFIG, LIVE, CURRENT...
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.