[libvirt] [PATCH v5 15/15] qemu_driver: improve comparison/baseline error reporting

[Jiri Denemark]

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 at 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