Re: [PATCH 1/3] machine: Factor CPU type invalidation out into helper

[Gavin Shan]

Hi Igor,

On 7/14/23 22:07, Igor Mammedov wrote:
> On Thu, 13 Jul 2023 15:45:00 +1000
> Gavin Shan <gshan@redhat.com> wrote:
>
> > The CPU type invalidation logic in machine_run_board_init() is
> > independent enough. Lets factor it out into helper validate_cpu_type().
> > Since we're here, the relevant comments are improved a bit.
> >
> > No functional change intended.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
> >   1 file changed, 43 insertions(+), 38 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index f0d35c6401..68b866c762 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1349,12 +1349,52 @@ out:
> >       return r;
> >   }
> >   
> > +static void validate_cpu_type(MachineState *machine)
> s/validate_cpu_type/is_cpu_type_valid or better is_cpu_type_supported
>
> Is it going to be reused elsewhere (otherwise I don't see much reason to move 
> code around)?

The logic of checking if the CPU type is supported is independent enough. It's
the only reason why I factored it out into a standalone helper here. It has
been explained in the commit log. Lets have an individual helper for this if
you don't have strong taste. With it, machine_run_board_init() looks a bit more
clean.

I don't have strong opinion about the function name. Shall we return 'bool'
with is_cpu_type_supported()? Something like below. The 'bool' return value
is duplicate to 'local_err' in machine_run_board_init(). So I think the
function validate_cpu_type(machine, errp) looks good to me. Igor, could you
please help to confirm?

static bool is_cpu_type_supported(MachineState *machine, Error **errp)
{
    bool supported = true;

    :

    if (!machine_class->valid_cpu_types[i]) {
        error_setg(errp, "Invalid CPU type: %s", machine->cpu_type));
        error_append_hint(errp, "The valid types are: %s", model);
        for (i = 1; machine_class->valid_cpu_types[i]; i++) {
            error_append_hint(errp, ", %s", model);
        }
        error_append_hint(errp, "\n");

        supported = false;
    }

    :

    return supported;
}

void machine_run_board_init(MachineState *machine, const char *mem_path, Error 
**errp)
{
    Error *local_err = NULL;

    :

    /* These two conditions are duplicate to each other! */
    if (!is_cpu_type_supported(machine, &local_err) && local_err) {
        error_propagate(errp, local_err);
    }

    :
}

> > +{
> > +    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> > +    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> > +    CPUClass *cc = CPU_CLASS(oc);
> > +    int i;
> > +
> > +    /*
> > +     * Check if the user-specified CPU type is supported when the valid
> > +     * CPU types have been determined. Note that the user-specified CPU
> > +     * type is given by '-cpu' option.
> > +     */
> > +    if (!machine->cpu_type || !machine_class->valid_cpu_types) {
> > +        goto out_no_check;
> no goto-s please

Ok. Will be dropped in next revision.

> > +    }
> > +
> > +    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > +        if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    if (!machine_class->valid_cpu_types[i]) {
> > +        /* The user-specified CPU type is invalid */
> > +        error_report("Invalid CPU type: %s", machine->cpu_type);
> > +        error_printf("The valid types are: %s",
> > +                     machine_class->valid_cpu_types[0]);
> > +        for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> > +            error_printf(", %s", machine_class->valid_cpu_types[i]);
> > +        }
> > +        error_printf("\n");
> > +
> > +        exit(1);
>
> since you are touching that,
> turn it in errp handling, in separate patch 1st
> and only then introduce your helper.

Right, it's a good idea. I will have a preparatory patch for it where
the error messages will be accumulated to @local_err and finally propagate
it to @errp of machine_run_board_init().

> > +    }
> > +
> > +    /* Check if CPU type is deprecated and warn if so */
> > +out_no_check:
> > +    if (cc && cc->deprecation_note) {
> > +        warn_report("CPU model %s is deprecated -- %s",
> > +                    machine->cpu_type, cc->deprecation_note);
> > +    }
> > +}
> >   
> >   void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
> >   {
> >       MachineClass *machine_class = MACHINE_GET_CLASS(machine);
> > -    ObjectClass *oc = object_class_by_name(machine->cpu_type);
> > -    CPUClass *cc;
> >   
> >       /* This checkpoint is required by replay to separate prior clock
> >          reading from the other reads, because timer polling functions query
> > @@ -1405,42 +1445,7 @@ void machine_run_board_init(MachineState *machine, const 
> > char *mem_path, Error *
> >           machine->ram = machine_consume_memdev(machine, machine->memdev);
> >       }
> >   
> > -    /* If the machine supports the valid_cpu_types check and the user
> > -     * specified a CPU with -cpu check here that the user CPU is supported.
> > -     */
> > -    if (machine_class->valid_cpu_types && machine->cpu_type) {
> > -        int i;
> > -
> > -        for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > -            if (object_class_dynamic_cast(oc,
> > -                                          machine_class->valid_cpu_types[i])) {
> > -                /* The user specificed CPU is in the valid field, we are
> > -                 * good to go.
> > -                 */
> > -                break;
> > -            }
> > -        }
> > -
> > -        if (!machine_class->valid_cpu_types[i]) {
> > -            /* The user specified CPU is not valid */
> > -            error_report("Invalid CPU type: %s", machine->cpu_type);
> > -            error_printf("The valid types are: %s",
> > -                         machine_class->valid_cpu_types[0]);
> > -            for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> > -                error_printf(", %s", machine_class->valid_cpu_types[i]);
> > -            }
> > -            error_printf("\n");
> > -
> > -            exit(1);
> > -        }
> > -    }
> > -
> > -    /* Check if CPU type is deprecated and warn if so */
> > -    cc = CPU_CLASS(oc);
> > -    if (cc && cc->deprecation_note) {
> > -        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> > -                    cc->deprecation_note);
> > -    }
> > +    validate_cpu_type(machine);
> >   
> >       if (machine->cgs) {
> >           /*

Thanks,
Gavin