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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 19.02.2016 01:43, John Ferlan wrote:
> 
> 
> 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@xxxxxxxxxxxxx>
>>>> ---
>>>>  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...

Thank you for pointing this out. Somehow I failed to realize
it and started to appreciate this rule only recently ) 

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

Ok.

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

Sure, this change deserves its own commit with all explanations in commit
message.

> 
> John
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]