Hi, The commit message suggests this patch is doing several things at once...
On Thu, Oct 01, 2026 at 22:36:23 -0400, Rorie Reyes wrote: > The qemu-cpu-model-comparison QMP command can return a > "responsible-properties" array identifying which CPU properties > caused an incompatibility result. Expose this through the JSON > monitor layer by adding an optional `responsibleProps` output > parameter (char ***) to qemuMonitorJSONGetCPUModelComparison(). > > When the caller passes a non-NULL pointer the array is populated > from the QMP response; passing NULL preserves the existing > behaviour so all current callers are unaffected until they opt in. This is the main thing. > The existing caller in qemu_monitor.c is updated to pass NULL so > the build continues to work. You don't really need to explicitly say this, it's the minimal change you have to do to make the code compile. > A GenericError response from the QMP command is intercepted and > re-reported as VIR_ERR_CPU_INCOMPATIBLE with the QEMU error > description. Assuming the JSON query was properly constructed, all > GenericErrors should indicate an incompatible CPU model due to QEMU > binary limitations (e.g. old binary does not recognize newer > features or models). This sounds completely unrelated and should go to a separate patch. Se below for more comments on this part. > The existing unit test is updated to pass the new argument and > extended with additional calls: one that verifies the > responsible-properties array is correctly populated from the QMP > response, one that verifies it is NULL when the key is absent from > the response, one that verifies it is NULL when the array is empty, > and cases for "identical", "subset", and "superset" results > (including a "type" responsible-property) to cover all > result-string mappings. I would only do the minimal change here and add new tests in a separate patch. > Signed-off-by: Rorie Reyes <[email protected]> > --- > src/qemu/qemu_monitor.c | 2 +- > src/qemu/qemu_monitor_json.c | 26 +++++- > src/qemu/qemu_monitor_json.h | 5 +- > tests/qemumonitorjsontest.c | 159 +++++++++++++++++++++++++++++++++-- > 4 files changed, 182 insertions(+), 10 deletions(-) > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 2c86cd1971..e3edc88814 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -3335,7 +3335,7 @@ qemuMonitorGetCPUModelComparison(qemuMonitor *mon, > > QEMU_CHECK_MONITOR(mon); > > - return qemuMonitorJSONGetCPUModelComparison(mon, cpu_a, cpu_b, result); > + return qemuMonitorJSONGetCPUModelComparison(mon, cpu_a, cpu_b, result, > NULL); qemuMonitorGetCPUModelComparison is just a tiny wrapper around qemuMonitorJSONGetCPUModelComparison, you should propagate the new parameter one layer up right away. That is, squash the second patch into this one. > } > > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 8e88dc9442..a895341b67 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -5689,7 +5689,8 @@ int > qemuMonitorJSONGetCPUModelComparison(qemuMonitor *mon, > virCPUDef *cpu_a, > virCPUDef *cpu_b, > - char **result) > + char **result, > + char ***responsibleProps) > { > g_autoptr(virJSONValue) model_a = NULL; > g_autoptr(virJSONValue) model_b = NULL; > @@ -5697,6 +5698,11 @@ qemuMonitorJSONGetCPUModelComparison(qemuMonitor *mon, > g_autoptr(virJSONValue) reply = NULL; > const char *data_result; > virJSONValue *data; > + virJSONValue *propsArray; > + > + *result = NULL; > + if (responsibleProps) > + *responsibleProps = NULL; > > if (!(model_a = qemuMonitorJSONMakeCPUModel(cpu_a, true, false)) || > !(model_b = qemuMonitorJSONMakeCPUModel(cpu_b, true, false))) > @@ -5711,6 +5717,13 @@ qemuMonitorJSONGetCPUModelComparison(qemuMonitor *mon, > if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) > return -1; > > + if (qemuMonitorJSONHasError(reply, "GenericError")) { > + virJSONValue *error = virJSONValueObjectGet(reply, "error"); > + virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s", > + qemuMonitorJSONStringifyError(error)); > + return -1; > + } If this really means the CPUs are incompatible rather then an error, you should propagate the info to the caller rather then reporting it as VIR_ERR_CPU_INCOMPATIBLE error here. The caller decides whether CPU incompatibility should be reported as an error or just as a specific return code. But we normally report unknown CPU models and features as errors so I don't think we should handle that differently in s390. That said, leaving just qemuMonitorJSONCheckError should be enough. And if not, the changes should definitely go into a separate patch. > + > if (qemuMonitorJSONCheckError(cmd, reply) < 0) > return -1; > > @@ -5723,6 +5736,17 @@ qemuMonitorJSONGetCPUModelComparison(qemuMonitor *mon, > } > > *result = g_strdup(data_result); If you want to gain some bonus points, you can add a new patch before this one that would refactor qemuMonitor*GetCPUModelComparison so that it doesn't reports the result as a string :-) In other words, move if (STREQ(result, "identical")) return VIR_CPU_COMPARE_IDENTICAL; if (STREQ(result, "superset")) return VIR_CPU_COMPARE_SUPERSET; from qemu_driver.c to the monitor code and get rid of the char **result parameter completely. > + > + propsArray = virJSONValueObjectGetArray(data, "responsible-properties"); > + > + if (responsibleProps && propsArray && > + virJSONValueArraySize(propsArray) > 0 && > + !(*responsibleProps = virJSONValueArrayToStringList(propsArray))) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Invalid 'responsible-properties' list in > query-cpu-model-comparison response")); This would overwrite the error already reported by virJSONValueArrayToStringList. Just return -1 here. > + return -1; > + } QEMU reports a special "type" property in the responsible-properties list if the incompatibility reason is not only a set of features that might be allowed to be disabled to get compatible CPUs, but if the models themselves are incompatible. Should we filter it out somehow to avoid reporting "type" as a missing feature? > + > return 0; > } > > diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index 63eded7ecb..9a3695d8e1 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -443,8 +443,9 @@ int > qemuMonitorJSONGetCPUModelComparison(qemuMonitor *mon, > virCPUDef *cpu_a, > virCPUDef *cpu_b, > - char **result) > - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); > + char **result, > + char ***responsibleProps) > + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); > > GHashTable * > qemuMonitorJSONGetCommandLineOptions(qemuMonitor *mon); > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c > index a963029c69..ee7123d4c8 100644 > --- a/tests/qemumonitorjsontest.c > +++ b/tests/qemumonitorjsontest.c Move the new tests into a separate patch, please. > @@ -2623,27 +2623,174 @@ > testQemuMonitorJSONqemuMonitorJSONGetCPUModelComparison(const void *opaque) > g_autoptr(virCPUDef) cpu_a = virCPUDefNew(); > g_autoptr(virCPUDef) cpu_b = virCPUDefNew(); > g_autofree char *result = NULL; > + g_auto(GStrv) responsibleProps = NULL; I would suggest defining a code block around each test ({ ... }) and move result and responsibleProps inside it so that they are automatically freed after each test. > > if (!(test = qemuMonitorTestNewSchema(data->xmlopt, data->schema))) > return -1; > > + cpu_a->model = g_strdup("cpu_a"); > + cpu_b->model = g_strdup("cpu_b"); > + > + /* incompatible, NULL responsibleProps — must still work */ > if (qemuMonitorTestAddItem(test, "query-cpu-model-comparison", > - "{\"return\":{\"result\":\"test\"}}") < 0) > + "{\"return\":{\"result\":\"incompatible\"," > + > "\"responsible-properties\":[\"prop_a\",\"prop_b\"]}}") < 0) > return -1; > > - cpu_a->model = g_strdup("cpu_a"); > - cpu_b->model = g_strdup("cpu_b"); > + if (qemuMonitorJSONGetCPUModelComparison(qemuMonitorTestGetMonitor(test), > + cpu_a, cpu_b, &result, NULL) < > 0) > + return -1; > + > + if (!result || STRNEQ(result, "incompatible")) { Use STRNEQ_NULLABLE throughout all this code instead (although ideally the string comparison would not be here at all if qemuMonitorJSONGetCPUModelComparison is refactored as I suggested earlier). ... > + /* superset with "type" prop — generation-mismatch error path */ > + if (qemuMonitorTestAddItem(test, "query-cpu-model-comparison", > + "{\"return\":{\"result\":\"superset\"," > + "\"responsible-properties\":[\"type\"]}}") < > 0) > + return -1; Is this even possible? From qapi/machine.json I would expect the "type" property to be only present for incompatible CPUs. ... Jirka
