[libvirt] [PATCHv2 10/11] qemu_capabilities: Introduce virQEMUCapsQMPBaselineCPUModel (baseline using QEMU)

[Jiri Denemark]

Drop "(baseline using QEMU)" from Subject to make it shorter.

On Mon, Jul 09, 2018 at 22:56:54 -0500, Chris Venteicher wrote:
> Baseline cpu model using QEMU/QMP query-cpu-model-baseline
> 
> query-cpu-model-baseline only compares two CPUModels so multiple
> exchanges are needed to evaluate more than two CPUModels.
> ---
>  src/qemu/qemu_capabilities.c | 85 ++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_capabilities.h |  4 ++
>  2 files changed, 89 insertions(+)

This code has nothing to do with QEMU capabilities, it should be
implemented in qemu_driver.c.

> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 6f8983384a..e0bf78fbba 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -5424,3 +5424,88 @@ virQEMUCapsStripMachineAliases(virQEMUCapsPtr qemuCaps)
>      for (i = 0; i < qemuCaps->nmachineTypes; i++)
>          VIR_FREE(qemuCaps->machineTypes[i].alias);
>  }
> +
> +
> +/* in:
> + *  cpus[0]->model = "z13-base";
> + *  cpus[0]->features[0].name = "xxx";
> + *  cpus[0]->features[1].name = "yyy";
> + *  ***
> + *  cpus[n]->model = "s390x";
> + *  cpus[n]->features[0].name = "xxx";
> + *  cpus[n]->features[1].name = "yyy";
> + *
> + * out:
> + *  *baseline->model = "s390x";
> + *  *baseline->features[0].name = "yyy";
> + *
> + * (ret==0) && (*baseline==NULL) if a QEMU rejects model name or baseline command
> + */
> +int
> +virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd,
> +                               virCPUDefPtr *cpus,
> +                               virCPUDefPtr *baseline)
> +{
> +    qemuMonitorCPUModelInfoPtr  model_baseline = NULL;
> +    qemuMonitorCPUModelInfoPtr  new_model_baseline = NULL;
> +    qemuMonitorCPUModelInfoPtr  next_model = NULL;

One space between the type and the variable name would be enough.

> +    bool migratable_only = true;
> +    int ret = -1;
> +    size_t i;
> +
> +    *baseline = NULL;
> +
> +    if (!cpus || !cpus[0] || !cpus[1]) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s", _("less than 2 cpus"));
> +        goto cleanup;
> +    }

I guess you're going to special case the single CPU use case in the
caller...

> +
> +    for (i = 0; !cpus[i]; i++) {      /* last element in cpus == NULL */

As already mentioned by Collin, this can't really work unless you remove
'!'.

> +        virCPUDefPtr cpu = cpus[i];
> +
> +        VIR_DEBUG("cpu[%lu]->model = %s", i, NULLSTR(cpu->model));
> +
> +        if (!(next_model = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) {
> +            virReportError(VIR_ERR_INVALID_ARG, "%s", _("cpu without content"));

This would override any error reported by
virQEMUCapsCPUModelInfoFromCPUDef.

> +            goto cleanup;
> +        }
> +
> +        if (i == 0) {
> +            model_baseline = next_model;
> +            continue;
> +        }

Alternatively you could just call virQEMUCapsCPUModelInfoFromCPUDef once
before the for loop to set model_baseline and start the for loop at
index 1.

> +
> +        if (qemuMonitorGetCPUModelBaseline(cmd->mon, model_baseline,
> +                                           next_model, &new_model_baseline) < 0)
> +            goto cleanup;
> +
> +        if (!new_model_baseline) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("QEMU doesn't support baseline or recognize model %s or %s"),
> +                           model_baseline->name,
> +                           next_model->name);
> +            ret = 0;

This doesn't make a lot of sense. It's a real error so why you'd return
0? And the error message should be reported by
qemuMonitorGetCPUModelBaseline.

> +            goto cleanup;
> +        }
> +
> +        qemuMonitorCPUModelInfoFree(model_baseline);
> +        qemuMonitorCPUModelInfoFree(next_model);
> +
> +        next_model = NULL;
> +
> +        model_baseline = new_model_baseline;
> +    }
> +
> +    if (!(*baseline = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, model_baseline)))
> +        goto cleanup;
> +
> +    VIR_DEBUG("baseline->model = %s", NULLSTR((*baseline)->model));

How could the model name be NULL at this point?

> +
> +    ret = 0;
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(model_baseline);
> +    qemuMonitorCPUModelInfoFree(next_model);
> +
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 7be636d739..d49c8b32ec 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -593,6 +593,10 @@ void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
>  qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef);
>  virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);
>  
> +int virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd,
> +                                   virCPUDefPtr *cpus,
> +                                   virCPUDefPtr *baseline);
> +
>  virFileCachePtr virQEMUCapsCacheNew(const char *libDir,
>                                      const char *cacheDir,
>                                      uid_t uid,

Jirka