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