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
