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

[Igor Mammedov]

On Tue, 18 Jul 2023 16:11:42 +1000
Gavin Shan <gshan@redhat.com> wrote:

> 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?

I'd check errp and drop bool return, otherwise looks fine to me

> 
> 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
>