Re: [libvirt] [PATCH v5 15/15] qemu_driver: improve comparison/baseline error reporting
On Thu, Sep 19, 2019 at 16:25:06 -0400, Collin Walling wrote:
Providing an erroneous CPU definition in the XML file provided to
the
hypervisor-cpu-compare/baseline command will result in a verbose
internal error. Let's add some sanity checking before executing the QMP
commands to provide a cleaner message if something is wrong with the
CPU model or features.
Signed-off-by: Collin Walling <walling(a)linux.ibm.com>
---
src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 153b2f2..6298c48 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13703,6 +13703,55 @@ qemuConnectCompareCPU(virConnectPtr conn,
}
+static int
+qemuConnectCheckCPUModel(qemuMonitorPtr mon,
+ virCPUDefPtr cpu,
+ bool reportError)
+{
+ qemuMonitorCPUModelInfoPtr modelInfo = NULL;
+ qemuMonitorCPUModelExpansionType type;
+ size_t i, j;
+ int ret = -1;
+
+ /* Collect CPU model name and features known by QEMU */
+ type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL;
+ if (qemuMonitorGetCPUModelExpansion(mon, type, cpu,
+ true, false, &modelInfo) < 0)
+ goto cleanup;
+
+ /* Sanity check CPU model */
+ if (!modelInfo) {
+ if (reportError)
+ virReportError(VIR_ERR_CPU_INCOMPATIBLE,
+ _("Unknown CPU model: %s"), cpu->model);
This is not really related to CPU compatibility. Except for taking a CPU
model (or feature below) from a newer QEMU and comparing it on an old
one. But this is indistinguishable from an incorrect input. For this
reason and for consistency among all architectures we should just report
this as a normal error (e.g., VIR_ERR_CONFIG_UNSUPPORTED).
+ goto cleanup;
+ }
+
+ /* Sanity check CPU features */
+ for (i = 0; i < cpu->nfeatures; i++) {
+ const char *feat = cpu->features[i].name;
+
+ for (j = 0; j < modelInfo->nprops; j++) {
+ const char *prop = modelInfo->props[j].name;
+ if (STREQ(feat, prop))
+ break;
+ }
+
+ if (j == modelInfo->nprops) {
+ if (reportError)
+ virReportError(VIR_ERR_CPU_INCOMPATIBLE,
+ _("Unknown CPU feature: %s"), feat);
Ditto.
+ goto cleanup;
+ }
+ }
+ ret = 0;
+
+ cleanup:
+ qemuMonitorCPUModelInfoFree(modelInfo);
+ return ret;
+}
+
+
static virCPUCompareResult
qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
const char *libDir,
@@ -13723,6 +13772,13 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
if (qemuProcessQMPStart(proc) < 0)
goto cleanup;
+ if (qemuConnectCheckCPUModel(proc->mon, cpu_a, failIncompatible) ||
+ qemuConnectCheckCPUModel(proc->mon, cpu_b, failIncompatible)) {
That said, you don't need to pass failIncompatible to
qemuConnectCheckCPUModel...
+ if (!failIncompatible)
+ ret = VIR_CPU_COMPARE_INCOMPATIBLE;
and you can drop this conditional.
+ goto cleanup;
+ }
+
if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, &result) <
0)
goto cleanup;
@@ -13913,6 +13969,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
if (qemuProcessQMPStart(proc) < 0)
goto cleanup;
+ if (qemuConnectCheckCPUModel(proc->mon, cpus[0], true))
+ goto cleanup;
+
if (VIR_ALLOC(baseline) < 0)
goto error;
@@ -13921,6 +13980,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
for (i = 1; i < ncpus; i++) {
+ if (qemuConnectCheckCPUModel(proc->mon, cpus[i], true))
+ goto error;
+
Also in all four cases you should use qemuConnectCheckCPUModel() < 0
check.
Jirka