[libvirt] [PATCH 27/34] conf: ABI: Split up and improve vcpu info ABI checking

[John Ferlan]

On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Extract the checking code into a separate function and prepare the
> infrastructure for checking the new structure type.
> ---
>  src/conf/domain_conf.c | 41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 631e1db..66fc6d3 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17835,6 +17835,35 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src,
>  }
> 
> 
> +static bool
> +virDomainDefVcpuCheckAbiStability(virDomainDefPtr src,
> +                                  virDomainDefPtr dst)

I see use of "Vcpu" here...

> +{
> +    size_t i;
> +
> +    if (src->maxvcpus != dst->maxvcpus) {

Should these be accessors?  Like they were in the moved code?

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Target domain vCPU max %zu does not match source %zu"),
> +                       dst->maxvcpus, src->maxvcpus);
> +        return false;
> +    }
> +
> +    for (i = 0; i < src->maxvcpus; i++) {

Allowing for this to be an accessor/local too.

> +        virDomainVCpuInfoPtr svcpu = &src->vcpus[i];
> +        virDomainVCpuInfoPtr dvcpu = &dst->vcpus[i];
> +
> +        if (svcpu->online != dvcpu->online) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("State of vCPU '%zu' differs between source and "
> +                             "destination definitions"), i);
> +            return false;
> +        }

This changes the original code/design just slightly from a counted value
online to having the same order between source/dest.  If the current
algorithm of using the first 'current' vcpus doesn't change, then I
foresee perhaps an interesting issue/question.

Say we start with 2 current (0,1) and 4 total (0,1,2,3). If we allow
someone to start/hotplug #3, then a migration occurs. Would the "target"
start "0,1,2" or "0,1,3"?

If I think about the current algorithm, it's get the # of vCPU's
"current" (virDomainDefGetVCpus) and then set online in order 0, 1, 2
(virDomainDefSetVCpus).

That causes a failure for this algorithm, but should it?   Again only an
issue if you're ultimate goal is to allow the user to choose which
vCPU's to place online or offline. I haven't looked that far forward yet.

Conditional ACK depending on response.

John


> +    }
> +
> +    return true;
> +}
> +
> +
>  /* This compares two configurations and looks for any differences
>   * which will affect the guest ABI. This is primarily to allow
>   * validation of custom XML config passed in during migration
> @@ -17908,18 +17937,8 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
>          goto error;
>      }
> 
> -    if (virDomainDefGetVCpus(src) != virDomainDefGetVCpus(dst)) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("Target domain vCPU count %d does not match source %d"),
> -                       virDomainDefGetVCpus(dst), virDomainDefGetVCpus(src));
> +    if (!virDomainDefVcpuCheckAbiStability(src, dst))
>          goto error;
> -    }
> -    if (virDomainDefGetVCpusMax(src) != virDomainDefGetVCpusMax(dst)) {
> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                       _("Target domain vCPU max %d does not match source %d"),
> -                       virDomainDefGetVCpusMax(dst), virDomainDefGetVCpusMax(src));
> -        goto error;
> -    }
> 
>      if (src->iothreads != dst->iothreads) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>