Re: [libvirt] [PATCHv2 10/11] qemu_capabilities: Introduce virQEMUCapsQMPBaselineCPUModel (baseline using QEMU)
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, _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
Re: [libvirt] [PATCHv2 10/11] qemu_capabilities: Introduce virQEMUCapsQMPBaselineCPUModel (baseline using QEMU)
On 07/09/2018 11:56 PM, 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(+)
>
> 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";
"s390x" is not a valid CPU model.
> + * 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;
> +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;
> +}
> +
> +for (i = 0; !cpus[i]; i++) { /* last element in cpus == NULL */
Remove the ! from the condition here.
> +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"));
> +goto cleanup;
> +}
> +
> +if (i == 0) {
> +model_baseline = next_model;
> +continue;
> +}
> +
> +if (qemuMonitorGetCPUModelBaseline(cmd->mon, model_baseline,
> + next_model, _model_baseline)
> < 0)
> +goto cleanup;
Hmmm... since we're fencing who can use baseline based on the architecture, I
wonder if it would
help to clean things up and have qemuMonitorJSONGetCPUModelBaseline return
nonzero if "GenericError"
is reported? That would allow you to remove the condition block below, or at
the very least clean up
the error message to say something like "Either model %s or %s is not
supported."
> +
> +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;
> +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));
> +
> +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,
>
--
Respectfully,
- Collin Walling
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
