Re: [Qemu-devel] [PATCH v2] s390: return unavailable features via query-cpu-definitions

[David Hildenbrand <david@redhat.com>]

>  
> +static S390CPUModel *get_max_cpu_model(Error **errp);
> +
>  #ifndef CONFIG_USER_ONLY
> +static void list_add_feat(const char *name, void *opaque);

Wonder if we should declare all these prototypes at the beginning of the
file.

> +
> +static void check_unavailable_features(const S390CPUModel *max_model,
> +                                       const S390CPUModel *model,
> +                                       strList **unavailable)
> +{
> +    S390FeatBitmap missing;
> +
> +    /* check general model compatibility */
> +    if (max_model->def->gen < model->def->gen ||
> +        (max_model->def->gen == model->def->gen &&
> +         max_model->def->ec_ga < model->def->ec_ga)) {
> +        list_add_feat("type", unavailable);
> +    }
> +
> +    /* detect missing features if any to properly report them */
> +    bitmap_andnot(missing, model->features, max_model->features,
> +                  S390_FEAT_MAX);
> +    if (!bitmap_empty(missing, S390_FEAT_MAX)) {
> +        s390_feat_bitmap_to_ascii(missing,
> +                                  unavailable,
> +                                  list_add_feat);

This certainly fits into one line.

> +    }
> +}
> +
> +struct CpuDefinitionInfoListData {
> +    CpuDefinitionInfoList *list;
> +    Error **errp;
> +};
> +
>  static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>  {
> -    CpuDefinitionInfoList **cpu_list = opaque;
> +    struct CpuDefinitionInfoListData *cpu_list_data = opaque;
> +    CpuDefinitionInfoList **cpu_list = &cpu_list_data->list;
>      CpuDefinitionInfoList *entry;
>      CpuDefinitionInfo *info;
>      char *name = g_strdup(object_class_get_name(klass));
>      S390CPUClass *scc = S390_CPU_CLASS(klass);
> +    Object *obj;
> +    S390CPU *sc;
> +    S390CPUModel *scm;
>  
>      /* strip off the -s390-cpu */
>      g_strrstr(name, "-" TYPE_S390_CPU)[0] = 0;
> @@ -300,21 +336,33 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>      info->migration_safe = scc->is_migration_safe;
>      info->q_static = scc->is_static;
>      info->q_typename = g_strdup(object_class_get_name(klass));
> -
> +    /* check for unavailable features */
> +    obj = object_new(object_class_get_name(klass));
> +    sc = S390_CPU(obj);
> +    scm = get_max_cpu_model(cpu_list_data->errp);

Hmmmm, if this function fails, we will create the same error multiple
times (as there is no way to stop this function from iterating). And we
will fail to create a cpu model list in case there is no host cpu model,
which is a change in behavior (as we will report an error).

Would it be better to simply get the max model in
arch_query_cpu_definitions() and pass it via CpuDefinitionInfoListData,
instead of the errp variable?

Then you could simply skip the checks and set
info->has_unavailable_features = false in case there is no max model
(get_max_cpu_model() returned NULL / an error). (same behavior as for now)

Errors from get_max_cpu_model() then should be ignored and not reported.



> +    if (scm && sc->model) {
> +        info->has_unavailable_features = true;
> +        check_unavailable_features(scm, sc->model, &info->unavailable_features);
> +    }
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;
>      entry->next = *cpu_list;
>      *cpu_list = entry;
> +    object_unref(obj);
>  }
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  {
> -    CpuDefinitionInfoList *list = NULL;
> +    struct CpuDefinitionInfoListData list_data = {
> +        .list = NULL,
> +        .errp = errp,
> +    };
>  
> -    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false, &list);
> +    object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
> +                         &list_data);
>  
> -    return list;
> +    return list_data.list;
>  }
>  
>  static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> 


-- 

Thanks,

David