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

[John Ferlan]

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.

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