[libvirt] [PATCH v4 5/8] qemu_monitor: implement query-cpu-model-comparison

[Jiri Denemark]

On Wed, Jul 24, 2019 at 18:18:27 +0200, Boris Fiuczynski wrote:
> On 7/17/19 4:03 PM, Collin Walling wrote:
> > Interfaces with QEMU to compare CPU models. The command takes two
> > CPU models, A and B, that are given a model name and an optional list
> > of CPU features. Through the query-cpu-model-comparison command issued
> > via QMP, a result is produced that contains the comparison evaluation
> > string (identical, superset, subset, incompatible) as well as a list
> > of properties (aka CPU features) responsible for the subset or
> > superset result.
> > 
> > Signed-off-by: Collin Walling <walling at linux.ibm.com>
> > Reviewed-by: Daniel Henrique Barboza <danielh413 at gmail.com>
> > ---
> >   src/qemu/qemu_monitor.c      | 22 ++++++++++
> >   src/qemu/qemu_monitor.h      |  9 +++++
> >   src/qemu/qemu_monitor_json.c | 95 ++++++++++++++++++++++++++++++++++++++++++++
> >   src/qemu/qemu_monitor_json.h | 10 +++++
> >   4 files changed, 136 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> > index 136d3e2..b16e149 100644
> > --- a/src/qemu/qemu_monitor.c
> > +++ b/src/qemu/qemu_monitor.c
> > @@ -3670,6 +3670,28 @@ qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> >   }
> >   
> >   
> > +int
> > +qemuMonitorGetCPUModelComparison(qemuMonitorPtr mon,
> > +                                 const char *model_a_name,
> > +                                 int model_a_nprops,
> > +                                 virCPUFeatureDefPtr model_a_props,
> > +                                 const char *model_b_name,
> > +                                 int model_b_nprops,
> > +                                 virCPUFeatureDefPtr model_b_props,
> > +                                 qemuMonitorCPUModelInfoPtr *model_result)
> > +{
> > +    VIR_DEBUG("model_a_name=%s model_a_nprops=%d model_b_name=%s model_b_nprops=%d",
> > +               model_a_name, model_a_nprops, model_b_name, model_b_nprops);
> > +
> > +    QEMU_CHECK_MONITOR(mon);
> > +
> > +    return qemuMonitorJSONGetCPUModelComparison(mon, model_a_name, model_a_nprops,
> > +                                                model_a_props, model_b_name,
> > +                                                model_b_nprops, model_b_props,
> > +                                                model_result);
> > +}
> > +
> > +
> >   void
> >   qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info)
> >   {
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index 4ec74f1..f0cf70a 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -1093,6 +1093,15 @@ int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon,
> >                                      virCPUFeatureDefPtr model_b_props,
> >                                      qemuMonitorCPUModelInfoPtr *model_result);
> >   
> > +int qemuMonitorGetCPUModelComparison(qemuMonitorPtr mon,
> > +                                     const char *model_a_name,
> > +                                     int model_a_nprops,
> > +                                     virCPUFeatureDefPtr model_a_props,
> > +                                     const char *model_b_name,
> > +                                     int model_b_nprops,
> > +                                     virCPUFeatureDefPtr model_b_props,
> > +                                     qemuMonitorCPUModelInfoPtr *model_result);
> > +
> >   qemuMonitorCPUModelInfoPtr
> >   qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);
> >   
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index b599ee6..3c33c63 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -5875,6 +5875,101 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
> >   }
> >   
> >   
> > +static int
> > +qemuMonitorJSONParseCPUModelPropName(size_t pos ATTRIBUTE_UNUSED,
> > +                                     virJSONValuePtr item,
> > +                                     void *opaque)
> > +{
> > +    if (!qemuMonitorJSONParseCPUModelProperty(virJSONValueGetString(item),
> > +                                              item, opaque))
> 
> This need to be changed into
> 
>      if (qemuMonitorJSONParseCPUModelProperty(virJSONValueGetString(item),
>                                               item, opaque) < 0)
> 
> > +        return -1;
> > +
> > +    virJSONValueFree(item);
> > +    return 0;
> > +}
> > +
> > +
> > +int
> > +qemuMonitorJSONGetCPUModelComparison(qemuMonitorPtr mon,
> > +                                     const char *model_a_name,
> > +                                     int model_a_nprops,
> > +                                     virCPUFeatureDefPtr model_a_props,
> > +                                     const char *model_b_name,
> > +                                     int model_b_nprops,
> > +                                     virCPUFeatureDefPtr model_b_props,
> > +                                     qemuMonitorCPUModelInfoPtr *model_result)
> > +{
> > +    int ret = -1;
> > +    virJSONValuePtr model_a;
> > +    virJSONValuePtr model_b = NULL;
> > +    virJSONValuePtr cmd = NULL;
> > +    virJSONValuePtr reply = NULL;
> > +    virJSONValuePtr data;
> > +    const char *result_name;
> > +    virJSONValuePtr props;
> > +    qemuMonitorCPUModelInfoPtr compare = NULL;
> > +
> > +    if (!(model_a = qemuMonitorJSONMakeCPUModel(model_a_name, model_a_nprops,
> > +                                                model_a_props, true)) ||
> > +        !(model_b = qemuMonitorJSONMakeCPUModel(model_b_name, model_b_nprops,
> > +                                                model_b_props, true)))
> > +        goto cleanup;
> > +
> > +    if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-comparison",
> > +                                           "a:modela", &model_a,
> > +                                           "a:modelb", &model_b,
> > +                                           NULL)))
> > +        goto cleanup;
> > +
> > +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
> > +        goto cleanup;
> > +
> > +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> 
> Did you test scenarios which contain unknown cpu model name or unknown 
> features?
> This caused errors for me during testing that resulted in e.g.
>   error : qemuMonitorJSONCheckError:415 : internal error: unable to 
> execute QEMU command 'query-cpu-model-comparison': Property '.boris' not 
> found
>   warning : virQEMUCapsLogProbeFailure:4586 : Failed to probe 
> capabilities for /usr/bin/qemu-system-s390x: internal error: unable to 
> execute QEMU command 'query-cpu-model-comparison': Property '.boris' not 
> found
> 
> If I think about a scenario like: I run "virsh domcapabilties" on a new 
> machine with new cpu model and new cpu features, new libvirt, new qemu 
> and use "virsh hypervisor-cpu-compare" with the output xml on my old 
> machine with old cpu &feature, older libvirt and older qemu
> I would expect to get as an answer: Incompatible
> 
> If my expectation to get incompatible is wrong please correct me but an 
> "internal error" is not what should occur.
> What is the qemu specification for this?

Actually returning error in such case is the correct way and also
consistent with how it works for x86. The only difference is we report
these errors ourselves for x86 while s390 would report the error from
QEMU. The QEMU error is not exactly nice, but it is correct. So adding
any code which would allow libvirt detect these errors and report better
error messages would be a nice enhancement, but it's not really
necessary.

Jirka