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