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

[Boris Fiuczynski]

On 7/25/19 8:26 PM, Collin Walling wrote:
>>> +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?
>>
>>
> 
> Two routes:
> 
> 1) Fix up the logic in virQEMUCapsCPUModelComparison
> 
> Please see my comments on patch 8.
> 
> 2) Change the error class in the QEMU QMP code
> 
> The QMP code in QEMU currently reports a "GenericError" whenever
> something goes wrong (such as an unknown CPU model or feature). A
> possible solution to this would be to alter that code to set a specific
> error class, have libvirt check for that specific error type in the
> JSON, then handle it appropriately to print a cleaner message.
> 
> There is one drawback: we would technically be reporting this error
> twice -- once in virsh, and another in the libvirt daemon (the internal
> error) unless we wanted to suppress it.
> 
> This is one solution without too much messy code.
> 

Or you prevent the generic error by not making the call at all in the 
case QEMU does not know CPU model or features. To be able to do this you 
either need lists of all known CPU models and features (preferable) or 
you need to ask QEMU if it knows the CPU model and features by use of 
other QMP commands that provide a consumable response.

I would not simply turn an QEMU internal error into an answer 
"incompatible" and I am not a fan of parsing thru QEMU internal errors 
that turns the answer into "incompatible".

Regarding patch 8: The option you outline in patch 8 returns "CPU is 
incompatible" for every QEMU error and not only "CPU model or feature" 
unknown. Therefore I think that it is not a good idea.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294