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

[Peter Krempa]

On Mon, Nov 23, 2015 at 17:41:05 -0500, John Ferlan wrote:
> 
> 
> 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?

Hmm, yes in this case they should be used. I reordered the patches.

> 
> > +        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"?

It should be 0,1,3. Later I'll introduce XML elements that will track
individual vCPUs and allow to transport their state accross to the
destination. Currently this is just preparation code for that and since
there currently is no way to create disjoint vCPU indexes this basically
does the same as the previous code, but in a less optimal fashion.

I can extract just the code as-is and bump the algorithm change to the
patch that will actually be adding this.

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

Looking forward wouldn't really help. The patches don't exist yet ;)

> 
> Conditional ACK depending on response.
> 

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151126/25137250/attachment-0001.sig>