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@xxxxxxxxxxxxx>
>> ---
>> 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.
>
> 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;
>>
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list