Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Gavin Shan]

Hi Phil,

On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:
> Per cpu_model_from_type() docstring (added in commit 445946f4dd):
>
>    * Returns: CPU model name or NULL if the CPU class doesn't exist
>
> We must check the return value in order to avoid surprises, i.e.:
>
>   $ qemu-system-arm -machine virt -cpu cortex-a9
>    qemu-system-arm: Invalid CPU model: cortex-a9
>    The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), 
> (null), (null), (null), (null), (null), (null), (null), max
>
> Add assertions when the call can not fail (because the CPU type
> must be registered).
>
> Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   cpu-target.c          | 1 +
>   hw/core/machine.c     | 5 +++++
>   target/ppc/cpu_init.c | 1 +
>   3 files changed, 7 insertions(+)
>
> diff --git a/cpu-target.c b/cpu-target.c
> index 5eecd7ea2d..b0f6deb13b 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer 
> user_data)
>       const char *typename = object_class_get_name(OBJECT_CLASS(data));
>       g_autofree char *model = cpu_model_from_type(typename);
>   
> +    assert(model);
>       if (cc->deprecation_note) {
>           qemu_printf("  %s (deprecated)\n", model);
>       } else {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fc239101f9..730ec10328 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState 
> *machine, Error **errp)
>           /* The user specified CPU type isn't valid */
>           if (!mc->valid_cpu_types[i]) {
>               g_autofree char *requested = 
> cpu_model_from_type(machine->cpu_type);
> +            assert(requested);
>               error_setg(errp, "Invalid CPU model: %s", requested);
>               if (!mc->valid_cpu_types[1]) {
>                   g_autofree char *model = cpu_model_from_type(
>                                                    mc->valid_cpu_types[0]);
> +                assert(model);
>                   error_append_hint(errp, "The only valid type is: %s\n", 
> model);
>               } else {
>                   error_append_hint(errp, "The valid models are: ");
>                   for (i = 0; mc->valid_cpu_types[i]; i++) {
>                       g_autofree char *model = cpu_model_from_type(
>                                                    mc->valid_cpu_types[i]);
> +                    if (!model) {
> +                        continue;
> +                    }

Shall we assert(model) for this case, to be consistent with other cases? :)

>                       error_append_hint(errp, "%s%s",
>                                         model,
>                                         mc->valid_cpu_types[i + 1] ? ", " : "");

Otherwise, the separator here need to be adjusted because it's uncertain that
mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.


> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 344196a8ce..58f0c1e30e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer 
> user_data)
>       }
>   
>       name = cpu_model_from_type(typename);
> +    assert(name);
>       qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
>       for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
>           PowerPCCPUAlias *alias = &ppc_cpu_aliases[i];

Thanks,
Gavin