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

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.

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