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

[John Ferlan]

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

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.

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

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
want the 'def' returned for a transient domain - what would it call? I
think that answer is virDomainObjGetDefs.  If so, then the comment could
be modified to indicate code paths that need to get the transient
definition should call virDomainObjGetDefs (yet another aptly named method).

John

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