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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
