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

[Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>]

On 03.07.2017 11:25, David Hildenbrand wrote:
> 
>>  
>> +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.
> 
Don't know either. Looking around in QEMU, forward declarations for
static functions seem to be used sparsely (only when needed). I could
have reordered the functions to get around without forward decls but
this would have obscured the change. Maybe defer and clean up in a
general cleanup/refactoring?
>> +
>> +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.
True, will change.
> 
>> +    }
>> +}
>> +
>> +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?Simplifies things, I like it.
> 
> 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.
> 
Just to be sure: you suggest that I should call error_free() after
calling get_max_cpu_model, in order to prevent that the QMP command
query-cpu-definitions fails, right?
> 
> 
>> +    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,
>>
> 
> 


-- 

Mit freundlichen Grüßen/Kind Regards
   Viktor Mihajlovski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294