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

[John Ferlan]

On 02/02/2016 08:04 AM, Nikolay Shirokovskiy wrote:
> 
> 
> 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.) 

Sorry for the delay, but other things were more important to do in the
highly preemptible work queue.

First off - please try to add a blank line around your responses - it's
a readability thing...

w/r/t this change - I'm probably over-thinking it; however, it's a
concept and area of the code which is tricky and sensitive.  There were
also 3 different "things" happening here - let's keep them singular.

Looking at the history, I see commit id '3d021381' split up the
virDomainLiveConfigHelperMethod to be two functions, with the second
being virDomainObjUpdateModificationImpact.  Prior to that the "if
(*flags & VIR_DOMAIN_AFFECT_CONFIG) {" had two sub-checks, the second of
which is what's primarily left in virDomainLiveConfigHelperMethod.

Anyway since this change is "standalone", please separate it from the
rest...  That is the domain_conf.c change should be it's own patch.


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

OK since it's the only path to call virDomainLiveConfigHelperMethod,
let's make it a separate patch to make it more obvious (add something to
the commit message, too)

That leaves the others calling virDomainObjUpdateModificationImpact
which should be a separate patch too.

Hope this makes sense,

John