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

[Gavin Shan]

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