On Mon, Jul 09, 2018 at 22:56:49 -0500, Chris Venteicher wrote:
> Send both model name and a set of features/properties to QEMU for
> expansion rather than just the model name.
> 
> Required to expand name+props models of the form computed by baseline
> into fully expanded (all props/features listed) output.

This patch is doing too much at once and is quite hard to review.

> ---
>  src/qemu/qemu_capabilities.c | 42 +++++++++++++++++-----
>  src/qemu/qemu_monitor.c      | 38 ++++++++++++++++----
>  src/qemu/qemu_monitor.h      |  5 ++-
>  src/qemu/qemu_monitor_json.c | 69 ++++++++++++++++++++++++------------
>  src/qemu/qemu_monitor_json.h |  7 ++--
>  tests/cputest.c              |  7 +++-
>  6 files changed, 122 insertions(+), 46 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3d78e2e29b..72ab012a78 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2343,23 +2343,32 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
>      qemuMonitorCPUModelInfoPtr modelInfo = NULL;
>      qemuMonitorCPUModelInfoPtr nonMigratable = NULL;
>      virHashTablePtr hash = NULL;
> -    const char *model;
> +    const char *model_name;

Why? If we really want to do this, it should go into a separate patch
explaining why it is needed.

>      qemuMonitorCPUModelExpansionType type;
>      virDomainVirtType virtType;
>      virQEMUCapsHostCPUDataPtr cpuData;
>      int ret = -1;
> +    int err = -1;

We usually call such variable 'rc' and leave it uninitialized.

>  
>      if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
>          return 0;
>  
>      if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
>          virtType = VIR_DOMAIN_VIRT_QEMU;
> -        model = "max";
> +        model_name = "max";
>      } else {
>          virtType = VIR_DOMAIN_VIRT_KVM;
> -        model = "host";
> +        model_name = "host";
>      }
>  
> +    if ((VIR_ALLOC(modelInfo) < 0) ||
> +        (VIR_ALLOC(nonMigratable) < 0))
> +        goto cleanup;
> +
> +    if ((qemuMonitorCPUModelInfoInit(model_name, modelInfo) < 0) ||
> +        (qemuMonitorCPUModelInfoInit(model_name, nonMigratable) < 0))
> +        goto cleanup;

Redundant parentheses. But anyway, VIR_ALLOC +
qemuMonitorCPUModelInfoInit combo should be replaced with a single
qemuMonitorCPUModelInfoNew function which would do both. As a bonus
point, you could never end up with a non-NULL modelInfo structure with
name == NULL.

> +
>      cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType);
>  
>      /* Some x86_64 features defined in cpu_map.xml use spelling which differ
> @@ -2372,16 +2381,31 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
>      else
>          type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
>  
> -    if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &modelInfo) 
> < 0)
> +    if ((err = qemuMonitorGetCPUModelExpansion(mon, type, true, modelInfo)) 
> < 0)
>          goto cleanup;
>  
> -    /* Try to check migratability of each feature. */
> -    if (modelInfo &&
> -        qemuMonitorGetCPUModelExpansion(mon, type, model, false,
> -                                        &nonMigratable) < 0)
> +    if (err == 1) {
> +        ret = 0;      /* Qemu can't do expansion 1, exit without error */
> +        goto cleanup; /* We don't have info so don't update cpuData->info */
> +    }

It's not very clear to me why qemuMonitorGetCPUModelExpansion needs to
report 1. A separate patch with an explanation would be better.

> +
> +    if ((err = qemuMonitorGetCPUModelExpansion(mon, type, false, 
> nonMigratable)) < 0)
>          goto cleanup;
>  
> -    if (nonMigratable) {
> +    /* Try to check migratability of each feature */
> +    /* Expansion 1 sets migratable features true
> +     * Expansion 2 sets migratable and non-migratable features true
> +     *             (non-migratable set true only in some archs like X86)
> +     *
> +     * If delta between Expansion 1 and 2 exists...
> +     * - both migratable and non-migratable features set prop->value = true
> +     * - migratable features set prop->migatable = VIR_TRISTATE_BOOL_YES
> +     * - non-migratable features set prop->migatable = VIR_TRISTATE_BOOL_NO
> +     */
> +    if (err == 0) {
> +        /* Expansion 2 succeded
> +         * Qemu expanded both migratable and nonMigratable features */

This comment seems redundant. It's pretty clear from the code that both
expansions were successful at this point.

> +
>          qemuMonitorCPUPropertyPtr prop;
>          qemuMonitorCPUPropertyPtr nmProp;
>          size_t i;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2d9297c3a7..91b946c8b4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3619,20 +3619,46 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr 
> cpu)
>  }
>  
>  
> +/*
> + * type static:
> + *   Expand to static base model + delta property changes
> + *   Returned model is invariant and migration safe
> + *
> + *   model_info->name = base model name
> + *   model_info->props = features to +/- to base model to achive model_name
> + *
> + * type full:
> + *   Expand model to enumerate all properties
> + *   Returned model isn't guaranteed to be invariant or migration safe.
> + *
> + *   model_info->name = base model name
> + *   model_info->props = features to +/- to empty set to achive model_name
> + *
> + * type static_full:
> + *   Expand to static base model + delta property changes (pass 0)
> + *   Expand model to enumerate all properties (pass 1)
> + *   Returned model is invariant and migration safe
> + *
> + *   model_info->name = base model name
> + *   model_info->props = features to +/- to empty set to achive model_name
> + *
> + * migratable_only:
> + *   true: QEMU excludes non-migratable features
> + *   false: QEMU includes non-migratable features for some archs like X86
> + */

Function description in our public API format and using real
qemuMonitorCPUModelExpansionType values would be a lot better.

>  int
>  qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
>                                  qemuMonitorCPUModelExpansionType type,
> -                                const char *model_name,
> -                                bool migratable,
> -                                qemuMonitorCPUModelInfoPtr *model_info)
> +                                bool migratable_only,

Why do you need to rename migratable as migratable_only? The migratable
bool selects whether the CPU model returned by QEMU has to be migratable
or not. In any case it would be a separate change.

> +                                qemuMonitorCPUModelInfoPtr model_info)

If you keep the double pointer there, you could remove some hacks and
comments to explain these hacks in the JSON monitor implementation. And
you wouldn't need to introduce qemuMonitorCPUModelInfoFreeContents.

>  {
>      VIR_DEBUG("type=%d model_name=%s migratable=%d",
> -              type, model_name, migratable);
> +              type, (model_info ? NULLSTR(model_info->name):"NULL"),

The function is not supposed to be called with model_info == NULL, the
check here is useless. Not to mention that if model_info was NULL,
qemuMonitorJSONGetCPUModelExpansion would segfault anyway.

And I believe the model_info->name can't be NULL either.

All this would of course change with the double pointer and additionally
this function should check a proper value was passed in.

> +              migratable_only);
>  
>      QEMU_CHECK_MONITOR(mon);
>  
> -    return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name,
> -                                               migratable, model_info);
> +    return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable_only, 
> model_info);
>  }
>  
>  
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 0b84a91fbc..6b4b527512 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1016,9 +1016,8 @@ typedef enum {
>  
>  int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
> -                                    const char *model_name,
> -                                    bool migratable,
> -                                    qemuMonitorCPUModelInfoPtr *model_info);
> +                                    bool migratable_only,
> +                                    qemuMonitorCPUModelInfoPtr model_info);
>  
>  void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
>  void qemuMonitorCPUModelInfoFreeContents(qemuMonitorCPUModelInfoPtr 
> model_info);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 90d43eee97..9b681f4592 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5390,8 +5390,11 @@ 
> qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model)
>          }
>      }
>  
> -    ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name,
> -                                          "a:props", &cpu_props, NULL));
> +    if (model->nprops > 0)
> +        ignore_value(virJSONValueObjectCreate(&model_json, "s:name", 
> model->name,
> +                                              "a:props", &cpu_props, NULL));
> +    else
> +        ignore_value(virJSONValueObjectCreate(&model_json, "s:name", 
> model->name, NULL));

Just don't fill in cpu_props at all and then you can use "A:props" to
cover both cases. Anyway, this can be done in a separate patch.

>  
>   cleanup:
>      virJSONValueFree(cpu_props);
> @@ -5440,38 +5443,52 @@ 
> qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
>      return model;
>  }
>  
> +
> +/* return:
> + * -1 - Execution Failure
> + *  0 - Success
> + *  1 - Qemu unable to do expansion leaving "model" unmodified
> + */
>  int
>  qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                      qemuMonitorCPUModelExpansionType type,
> -                                    const char *model_name,
> -                                    bool migratable,
> -                                    qemuMonitorCPUModelInfoPtr *model_info)
> +                                    bool migratable_only,
> +                                    qemuMonitorCPUModelInfoPtr model)
>  {
>      int ret = -1;
> -    virJSONValuePtr model = NULL;
> -    virJSONValuePtr props = NULL;
> +    virJSONValuePtr json_model = NULL;
>      virJSONValuePtr cmd = NULL;
>      virJSONValuePtr reply = NULL;
>      virJSONValuePtr data;
>      virJSONValuePtr cpu_model;
> +    qemuMonitorCPUModelInfoPtr expanded_model = NULL;
> +    qemuMonitorCPUModelInfoPtr model_info = NULL;
>      const char *typeStr = "";
>  
> -    *model_info = NULL;
> +    if (!(model_info = qemuMonitorCPUModelInfoCopy(model)))
> +        return -1;
> +
> +    qemuMonitorCPUModelInfoFreeContents(model);

We usually don't touch output parameters until we know the function
succeeded. Double pointer would let you comply with this.

>  
> -    if (!(model = virJSONValueNewObject()))
> -        goto cleanup;
> +    if (!migratable_only) {
> +        /* Add property to input CPUModelInfo causing QEMU to include
> +         * non-migratable properties for some architectures like X86 */

This comment doesn't really belong here. It's the caller's decision to
run this command with appropriate arguments for each architecture.

>  
> -    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
> -        goto cleanup;
> +        qemuMonitorCPUProperty prop;
> +        prop.type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +        prop.value.boolean = false;
> +        prop.migratable = false;
> +
> +        if (VIR_STRDUP(prop.name, "migratable") < 0)
> +            goto cleanup;
>  
> -    if (!migratable) {
> -        if (!(props = virJSONValueNewObject()) ||
> -            virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 
> ||
> -            virJSONValueObjectAppend(model, "props", props) < 0)
> +        if (VIR_APPEND_ELEMENT(model_info->props, model_info->nprops, prop) 
> < 0)

You'd leak prop.name here.

>              goto cleanup;
> -        props = NULL;
>      }
>  
> +    if (!(json_model = qemuMonitorJSONBuildCPUModelInfoToJSON(model_info)))
> +        goto cleanup;
> +
>   retry:
>      switch (type) {
>      case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC:
> @@ -5486,7 +5503,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>  
>      if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion",
>                                             "s:type", typeStr,
> -                                           "a:model", &model,
> +                                           "a:model", &json_model,
>                                             NULL)))
>          goto cleanup;
>  
> @@ -5498,7 +5515,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>       * guest architecture or it is not supported in the host environment.
>       */
>      if (qemuMonitorJSONHasError(reply, "GenericError")) {
> -        ret = 0;
> +        ret = 1;
>          goto cleanup;
>      }
>  
> @@ -5517,7 +5534,9 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>       * on the result of the initial "static" expansion.
>       */
>      if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) {
> -        if (!(model = virJSONValueCopy(cpu_model)))
> +        virJSONValueFree(json_model);
> +
> +        if (!(json_model = virJSONValueCopy(cpu_model)))
>              goto cleanup;
>  
>          virJSONValueFree(cmd);
> @@ -5526,16 +5545,20 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> mon,
>          goto retry;
>      }
>  
> -    if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
> +    if (!(expanded_model = 
> qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
>          goto cleanup;
>  
> +    *model = *expanded_model; /* overwrite contents */
> +
>      ret = 0;
>  
>   cleanup:
> +    VIR_FREE(expanded_model); /* Free structure but not reused contents */
> +    qemuMonitorCPUModelInfoFreeContents(model_info);
> +
>      virJSONValueFree(cmd);
>      virJSONValueFree(reply);
> -    virJSONValueFree(model);
> -    virJSONValueFree(props);
> +    virJSONValueFree(json_model);
>      return ret;
>  }
>  
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 73e1cb6ace..9950483c5c 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -362,10 +362,9 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>  
>  int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>                                          qemuMonitorCPUModelExpansionType 
> type,
> -                                        const char *model_name,
> -                                        bool migratable,
> -                                        qemuMonitorCPUModelInfoPtr 
> *model_info)
> -    ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5);
> +                                        bool migratable_only,
> +                                        qemuMonitorCPUModelInfoPtr 
> model_info)
> +    ATTRIBUTE_NONNULL(4);
>  
>  int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
>                                         qemuMonitorCPUModelInfoPtr model_a,
> diff --git a/tests/cputest.c b/tests/cputest.c
> index baf2b3c648..27727aa29e 100644
> --- a/tests/cputest.c
> +++ b/tests/cputest.c
> @@ -495,9 +495,14 @@ cpuTestMakeQEMUCaps(const struct data *data)
>      if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
>          goto error;
>  
> +    if ((VIR_ALLOC(model) < 0) ||
> +        (qemuMonitorCPUModelInfoInit("host", model) < 0))
> +        goto cleanup;
> +
> +

One empty line would be enough.

>      if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
>                                          
> QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
> -                                        "host", true, &model) < 0)
> +                                        true, model) < 0)
>          goto error;
>  
>      if (!(qemuCaps = virQEMUCapsNew()))

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to