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

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(a)virtuozzo.com&gt; >> --- >> 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list