Re: [libvirt] [PATCH 1/3] domain: reuse update flags checking functions

On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote: > 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)) { 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].

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...

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...