[libvirt] [PATCH 3/3] common-impl: make virDomainObjGetPersistentDef return only persistent config

Sun Feb 21 13:22:32 UTC 2016

On 19.02.2016 02:19, John Ferlan wrote:
> 
> 
> On 02/02/2016 08:18 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 02.02.2016 15:13, John Ferlan wrote:
>>>
>>>
>>> On 01/15/2016 09:05 AM, Nikolay Shirokovskiy wrote:
>>>> It's rather unexpected that function with such a name could return
>>>> transient definition. This is possible if we call it for
>>>> a transient active domain. But if it is called in such a conditions?
>>>> No. So we would better fix this case to return NULL.
>>>>
>>>> Calling conditions investigation.
>>>>
>>>> There are 4 distinct callers:
>>>>
>>>> 1. migration - explicitly set 'persistent' before calling.
>>>> 2. libxl_driver
>>>> First it assures that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false.
>>>> Then all usages of definition is under VIR_DOMAIN_VCPU_CONFIG flag,
>>>> so 'persistent' is set.
>>>>
>>>> 3. virDomainObjCopyPersistentDef - all callers of this function
>>>> has next structure:
>>>> 1 call virDomainLiveConfigHelperMethod or virDomainObjUpdateModificationImpact
>>>> thus we are sure that (VIR_DOMAIN_VCPU_CONFIG && !persistent) is false.
>>>> 2. call function of interest under VIR_DOMAIN_VCPU_CONFIG, so
>>>> 'persistent' flag is set.
>>>>
>>>> 4. virDomainLiveConfigHelperMethod - the same reason as in 3.
>>>>
>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>>>> ---
>>>>  src/conf/domain_conf.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>
>>> You've based your assumptions off of the change you made in 1/3 to
>>> virDomainObjUpdateModificationImpact regarding combining that if
>>> statement.  Also even if the following would fail, you would have need
>>> to provide some sort of virReportError.
>> But that change is merely a refactoring it does not change behaviour.
>> I'll add an error message of course but first I'll wait for agreement on that 
>> this patch is correct. May be I miss something as I really don't 
>> see why change you mention is matter.
> 
> Again sorry for the delay...
> 
> I have a suspicion that there's a "hidden" meaning/side-effect to
> calling this... The whole usage of "newDef" vs "def" needs better
> nomenclature to stick in my long term memory. To refresh my short term
> though... The 'newDef' is the "Next" definition to activate at shutdown
> (of a running domain), while 'def' is currently active definition
> (whether it's running or not).

Yes, it quite mangled place. I think we'd better stick with
next definitions as this is how we seem to use them in other places.

1. transient config - config for a running domain, lost after it is
destroyed.
2. persistent config - config on disk for a domain.

Looking at the code I've found next correnspondence of 
def and newDef to this definitions:

1. active persistent domain
  def - transient
  newDef - persistent
2. inactive persistent domain
  def - persistent
3. transient domain (active)
  def - transient

Thus newDef has only one purpose - persistent config of active persistent domain.
def could be transient or persistent depends on situation. Now
one can understand the reason for the function virDomainObjSetDefTransient
naming. On persistent domain start we move from 'persistent' to 'transient'
for 'def' meaining. 'live' flags helps us do it before actual start
is happend.

> 
> Perhaps the function is less than adequately named considering that
> Persistent usually refers to on disk, although in this case, I'm
> wondering if it means whatever is active for the domain without having
> to have the caller know whether it's a "running" and whether it's taking
> it from disk.

I think such a function would be hard to use as usually we need to 
distinguish transient/persistent and have no notion of 'active'.

Hmm ... if someone knows it is transient domain or inactive persistent
domain it can refer to config without specifying it further as
transient or persistent. Well in this case he can just use
plain 'def' instead of this function.

Another common usage of def is the case we need 'transient' config.
It is always in 'def'. This is why we don't have virDomainObjGetTransientDef.

> 
> In the case of a transient domain, the persistent domain is only valid
> when "running" and there's no need for 'newDef', hence why
> virDomainObjSetDefTransient will make that check (and why
> virDomainObjGetPersistentDef will call it).

Yes that check is there because there is no need for newDef for a transient domain.
But I think the reason virDomainObjGetPersistentDef calls virDomainObjSetDefTransient
is different. 'Set' function is kind of lazy initialization
tool. We initialize newDef only when someone wants change
persistent config for a running persistent domain for the first time.
This is the reason this check is there:
    if(domain->newDef)                                                                                             
        return 0;     

However I doubt we need this lazy initialization there anymore.
After cb4c2694f166beca8f3e1fb37dedeec1ff3fbdf6 commit newDef is
initialized early in the process of starting of domain for
lxc and qemu drivers at least.

> 
> I didn't follow in depth the instances you pointed out, but perhaps they
> are all that's left using the Get function. Maybe in the time since the
> Get function was created and now there's been code that's remove calls
> to the Get function and thus we've reached a point where we could do
> what you propose, but I haven't spent cycles researching all that. My
> point is - it's not only the current callers - what's the recent history
> on this? Has anything that used to call it now no longer call it and
> what was the reason?  A caller in a more general code path that would

It will take time to investigate it...

> want the 'def' returned for a transient domain - what would it call? I
> think that answer is virDomainObjGetDefs.  If so, then the comment could

Just get here. virDomainObjGetDefs has the exact description
of correspondence I wrote above in one place. Here it is 'live'
config instead of 'transient'.

> be modified to indicate code paths that need to get the transient
> definition should call virDomainObjGetDefs (yet another aptly named method).

Yes, if someone needs 'def' for transient domain - it needs 'transient' config
and can use this function. Hmm, one asks why bother to mention 'transient' on
config for transient domain as there is no other one. Well looks like
we have to as we have one type for all three states - active persistent,
active transient, inactive persistent.

As to reporting error upon return NULL from the function. I think it is
not right as if someone use the return value in this circumstances 
- it is a case of incorrect code and moreover it will crash instanly.

After all this investigation I would like to see one day all usages of 
def and newDef would be replaced by getTransient and getPersistent calls ))
with all the mechanics incapsulated.

Nikolay.

>>>
>>> John
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index e54c097..018c77e 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -2825,18 +2825,21 @@ virDomainObjSetDefTransient(virCapsPtr caps,
>>>>  
>>>>  /*
>>>>   * Return the persistent domain configuration. If domain is transient,
>>>> - * return the running config.
>>>> + * return NULL.
>>>>   *
>>>>   * @param caps pointer to capabilities info
>>>>   * @param xmlopt pointer to XML parser configuration object
>>>>   * @param domain domain object pointer
>>>> - * @return NULL on error, virDOmainDefPtr on success
>>>> + * @return NULL on error or transient domains, virDOmainDefPtr on success
>>>>   */
>>>>  virDomainDefPtr
>>>>  virDomainObjGetPersistentDef(virCapsPtr caps,
>>>>                               virDomainXMLOptionPtr xmlopt,
>>>>                               virDomainObjPtr domain)
>>>>  {
>>>> +    if (!domain->persistent)
>>>> +        return NULL;
>>>> +
>>>>      if (virDomainObjSetDefTransient(caps, xmlopt, domain, false) < 0)
>>>>          return NULL;
>>>>  
>>>>