[libvirt] [PATCHv2 03/11] qemu_monitor: Indicate when CPUModelInfo props report migratablity

[Chris Venteicher]

Quoting Collin Walling (2018-07-11 18:25:00)
> Not sure if I agree with the renaming here.
> 
> From what I understand, "if a cpu property is not migratable, then the 
> cpu model is also not migratable". 
> 
> So I think the original naming scheme is fine.

Very much welcome the feedback here from folks with experience...

Got to say though as a newbie I spent a crazy amount of time trying to sort out 
what migratability in qemuMonitorCPUModelInfo and migratable in 
qemuMonitorCPUProperty actually mean.

qemuMonitorCPUModelInfo looks like it should just be a libvirt structure 
encapsulating the CPUModelInfo in QMP messages.

But the CPUModelInfo in QMP messages doesn't contain migratability or 
migratable.

Turns out migratability and (prop) migratable are something libvirt overloads in 
CPUModelInfo with that's only used when storing host cpu capabilities in 
virQEMUCapsHostCPUData.

Moreover, the mechanism that virQEMUCapsProbeQMPHostCPU uses to get QMP to 
enumerate both migratable and non-migratable Host CPU properties only seems to 
work for some archs like X86. Other archs will only enumerate the migratable 
host properties.

So as best I can tell the "migratablity" variable in the qemuMonitorCPUModelInfo 
structure only means we were able to enumerate both migratable and 
non-migratable properties of the Host CPU (only when stored in 
virQEMUCapsHostCPUData) on a limited set of arch like X86.

In fact if Host CPU's "migratability" is set to true it likely means the Host 
CPU's CPUModelInfo contains non-migratable properties in addition to migratable 
properties.

So that is what I was trying to get at renaming 'migratability' to 
'props_migratable_valid'.  Trying to clarify that in the HostCPU case we were 
able to enumerate some non-migratable features and correspondingly indicate what 
properties are and are not migratable in the Host CPU.

With that said I would be glad to defer to those with libvirt experience here to 
tell me if I missed something, we should not change the name, or some other 
additional cleanup is in order.

> On 07/09/2018 11:56 PM, Chris Venteicher wrote:
> > Renamed variable in CPUModelInfo such that
> > props_migratable_valid is true when properties in CPUModelInfo
> > have been updated to accurately indicate if property is / isn't
> > migratable.
> > 
> > Property migratability is not returned directly in QMP messages but
> > rather is sometimes calculated within Libvirt by other means and then
> > stored in CPUModelInfo properties by Libvirt. props_migratable_valid is
> > set to true when this calculation has been done by Libvirt.
> > ---
> >  src/qemu/qemu_capabilities.c | 10 +++++-----
> >  src/qemu/qemu_monitor.c      |  2 +-
> >  src/qemu/qemu_monitor.h      |  2 +-
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index c7da916f9a..3d78e2e29b 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -2410,7 +2410,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> >              }
> >          }
> >  
> > -        modelInfo->migratability = true;
> > +        modelInfo->props_migratable_valid = true;
> >      }
> >  
> >      VIR_STEAL_PTR(cpuData->info, modelInfo);
> > @@ -2465,7 +2465,7 @@ virQEMUCapsGetCPUFeatures(virQEMUCapsPtr qemuCaps,
> >      }
> >  
> >      VIR_STEAL_PTR(*features, list);
> > -    if (migratable && !data->info->migratability)
> > +    if (migratable && !data->info->props_migratable_valid)
> >          ret = 1;
> >      else
> >          ret = 0;
> > @@ -2864,7 +2864,7 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps,
> >      virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type);
> >      int ret = 1;
> >  
> > -    if (migratable && cpuData->info && !cpuData->info->migratability)
> > +    if (migratable && cpuData->info && !cpuData->info->props_migratable_valid)
> >          return 1;
> >  
> >      if (ARCH_IS_S390(qemuCaps->arch)) {
> > @@ -3047,7 +3047,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> >                         _("invalid migratability value for host CPU model"));
> >          goto cleanup;
> >      }
> > -    hostCPU->migratability = val == VIR_TRISTATE_BOOL_YES;
> > +    hostCPU->props_migratable_valid = val == VIR_TRISTATE_BOOL_YES;
> >      VIR_FREE(str);
> >  
> >      ctxt->node = hostCPUNode;
> > @@ -3540,7 +3540,7 @@ virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> >      virBufferAsprintf(buf,
> >                        "<hostCPU type='%s' model='%s' migratability='%s'>\n",
> >                        typeStr, model->name,
> > -                      model->migratability ? "yes" : "no");
> > +                      model->props_migratable_valid ? "yes" : "no");
> >      virBufferAdjustIndent(buf, 2);
> >  
> >      for (i = 0; i < model->nprops; i++) {
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index a3278c018e..371aaa15da 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -3671,7 +3671,7 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig)
> >      if (VIR_STRDUP(copy->name, orig->name) < 0)
> >          goto error;
> >  
> > -    copy->migratability = orig->migratability;
> > +    copy->props_migratable_valid = orig->props_migratable_valid;
> >      copy->nprops = orig->nprops;
> >  
> >      for (i = 0; i < orig->nprops; i++) {
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index 18b59be985..208a7f5d21 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -1005,7 +1005,7 @@ struct _qemuMonitorCPUModelInfo {
> >      char *name;
> >      size_t nprops;
> >      qemuMonitorCPUPropertyPtr props;
> > -    bool migratability;
> > +    bool props_migratable_valid;
> >  };
> >  
> >  typedef enum {
> > 
> 
> 
> -- 
> Respectfully,
> - Collin Walling
>