Re: [PATCH v7 3/8] machine: Print CPU model name instead of CPU type

[Gavin Shan <gshan@redhat.com>]

Hi Phil,

On 11/28/23 20:55, Philippe Mathieu-Daudé wrote:
> On 27/11/23 00:12, Gavin Shan wrote:
>> The names of supported CPU models instead of CPU types should be
>> printed when the user specified CPU type isn't supported, to be
>> consistent with the output from '-cpu ?'.
>>
>> Correct the error messages to print CPU model names instead of CPU
>> type names.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/core/machine.c | 21 +++++++++++++++------
>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 05e1922b89..898c25552a 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1392,6 +1392,7 @@ static void is_cpu_type_supported(const MachineState *machine, Error **errp)
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>       ObjectClass *oc = object_class_by_name(machine->cpu_type);
>>       CPUClass *cc;
>> +    char *model;
>>       int i;
>>       /*
>> @@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState *machine, Error **errp)
>>           /* The user specified CPU type isn't valid */
>>           if (!mc->valid_cpu_types[i]) {
>> -            error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
>> +            model = cpu_model_from_type(machine->cpu_type);
>> +            g_assert(model != NULL);
>> +            error_setg(errp, "Invalid CPU type: %s", model);
>> +            g_free(model);
> 
>    g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
>    error_setg(errp, "Invalid CPU type: %s", requested);
> 

Yes, g_autofree shall be used here. Besides, "Invalid CPU type" needs to
be "Invalid CPU model".

>> +
>> +            model = cpu_model_from_type(mc->valid_cpu_types[0]);
>> +            g_assert(model != NULL);
>>               if (!mc->valid_cpu_types[1]) {
>> -                error_append_hint(errp, "The only valid type is: %s",
>> -                                  mc->valid_cpu_types[0]);
>> +                error_append_hint(errp, "The only valid type is: %s", model);
> 
>    g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[0]);
>    error_append_hint(errp, "The only valid type is: %s\n", model);
> 

Yes, as above.

>>               } else {
>> -                error_append_hint(errp, "The valid types are: %s",
>> -                                  mc->valid_cpu_types[0]);
>> +                error_append_hint(errp, "The valid types are: %s", model);
> 
> Please move all the enumeration in this ladder, this makes the logic
> simpler to follow:
> 
>    error_append_hint(errp, "The valid types are: ");
>    for (i = 0; mc->valid_cpu_types[i]; i++) {
>         g_autofree char *model =
>                             cpu_model_from_type(mc->valid_cpu_types[i]);
>         error_append_hint(errp, ", %s", model);
>    }
>    error_append_hint(errp, "\n");
> 

Yes, but we still need to ensure mc->valid_cpu_types[0] != NULL in advance.
"The valid types are: " needs to be "The valid models are: ". Besides,
your proposed code needs to be adjusted a bit like below. Otherwise, we
will get output "The valid types are: , aaa, bbb"

             } else {
                 error_append_hint(errp, "The valid types are: ");
                 for (i = 0; mc->valid_cpu_types[i]; i++) {
                     error_append_hint(errp, "%s%s",
                                       mc->valid_cpu_types[i],
                                       mc->valid_cpu_types[i + 1] ? ", " : "");
                 }
                 error_append_hint(errp, "\n");
             }

I will have separate PATCH[v8 3/9] to have the changes, together with the
precise hint when only mc->valid_cpu_types[0] is valid.

>>               }
>> +            g_free(model);
>>               for (i = 1; mc->valid_cpu_types[i]; i++) {
>> -                error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
>> +                model = cpu_model_from_type(mc->valid_cpu_types[i]);
>> +                g_assert(model != NULL);
>> +                error_append_hint(errp, ", %s", model);
>> +                g_free(model);
>>               }
>>               error_append_hint(errp, "\n");

Thanks,
Gavin