[libvirt] [PATCH v2 24/33] qemu: Use full CPU model expansion on x86

[Jiri Denemark]

On Tue, Feb 21, 2017 at 23:11:54 -0500, John Ferlan wrote:
> 
> 
> On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> > The static CPU model expansion is designed to return only canonical
> > names of all CPU properties. TO maintain backward compatibility libvirt
> 
> s/TO/To
> 
> > is stuck with different spelling of some of the features, which is only
> > returned by the full expansion. But in addition to returned all spelling
> 
> s/returned/returning
> 
> > variants for all properties the full expansion will contain properties
> > which are not guaranteed to be migration compatible. We need to combine
> > both expansions. First we need to call the static expansion to limit the
> > result to migratable properties. Then we can use the result of the
> > static expansion as an input to the full expansion to get both canonical
> > names and their aliases.
...
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index dd7907482..0454571c1 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -5026,7 +5026,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> >                                      qemuMonitorCPUModelInfoPtr *model_info)
> >  {
> >      int ret = -1;
> > -    virJSONValuePtr model;
> > +    virJSONValuePtr model = NULL;
> >      virJSONValuePtr cmd = NULL;
> >      virJSONValuePtr reply = NULL;
> >      virJSONValuePtr data;
> > @@ -5038,16 +5038,24 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> >  
> >      *model_info = NULL;
> >  
> > -    if (!(model = virJSONValueNewObject()))
> > -        goto cleanup;
> > + retry:
> > +    if (!model) {
> > +        if (!(model = virJSONValueNewObject()))
> > +            goto cleanup;
> >  
> > -    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
> > -        goto cleanup;
> > +        if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
> > +            goto cleanup;
> > +    }
> >  
> >      switch (type) {
> >      case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC:
> > +    case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL:
> >          typeStr = "static";
> >          break;
> > +
> > +    case QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL:
> > +        typeStr = "full";
> > +        break;
> >      }
> >  
> >      if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion",
> > @@ -5084,6 +5092,16 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> >          goto cleanup;
> >      }
> >  
> > +    if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) {
> > +        if (!(model = virJSONValueCopy(cpu_model)))
> > +            goto cleanup;
> > +
> > +        virJSONValueFree(cmd);
> > +        virJSONValueFree(reply);
> > +        type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL;
> > +        goto retry;
> 
> When you get here, model must be set.. The retry label tests for not
> set, which cannot be true - so why would the retry label be on the
> switch statement?  If it did move, then the move of the AppendString
> inside the "if" wouldn't be necessary.

Sure, no idea what I was thinking about :-)

> > +    }
> > +
> 
> This just seems odd - it's not really a retry, it's like piling on. To
> me retry is like trying again because something failed. In this case you
> get static, but then add on the full afterwards. I don't have a better
> suggestion for a label name.

I used "retry" since it's a common name for backward jumps. Anyway I
think a small comment explaining why we are jumping back will help...

Jirka