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

[Nikolay Shirokovskiy]

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