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

[Gavin Shan <gshan@redhat.com>]

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