Re: [PATCH-for-6.1? v2 2/3] hw/core: Have MachineClass::smp_parse() return boolean on error

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/08/21 13:26, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules"),
> return a boolean value indicating an error is set or not.
> Directly pass errp as the local_err is not requested in our
> case.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

This was intentional; no one should invoke mc->smp_parse directly except 
for machine_set_smp, therefore if mc->smp_parse returns bool there are 
more places where things can go wrong (instead of just the one that is 
fixed by patch 3).

Paolo

> ---
>   include/hw/boards.h |  2 +-
>   hw/core/machine.c   | 12 +++++++-----
>   hw/i386/pc.c        | 10 ++++++----
>   3 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index accd6eff35a..d5b7058c2e2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -209,7 +209,7 @@ struct MachineClass {
>       void (*reset)(MachineState *state);
>       void (*wakeup)(MachineState *state);
>       int (*kvm_type)(MachineState *machine, const char *arg);
> -    void (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
> +    bool (*smp_parse)(MachineState *ms, SMPConfiguration *config, Error **errp);
>   
>       BlockInterfaceType block_default_type;
>       int units_per_default_bus;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index abaeda589b7..159c6b098e2 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -743,7 +743,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
>       }
>   }
>   
> -static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> +static bool smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>   {
>       unsigned cpus    = config->has_cpus ? config->cpus : 0;
>       unsigned sockets = config->has_sockets ? config->sockets : 0;
> @@ -752,7 +752,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>   
>       if (config->has_dies && config->dies != 0 && config->dies != 1) {
>           error_setg(errp, "dies not supported by this machine's CPU topology");
> -        return;
> +        return true;
>       }
>   
>       /* compute missing values, prefer sockets over cores over threads */
> @@ -778,14 +778,14 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>                      "sockets (%u) * cores (%u) * threads (%u) < "
>                      "smp_cpus (%u)",
>                      sockets, cores, threads, cpus);
> -        return;
> +        return true;
>       }
>   
>       ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
>   
>       if (ms->smp.max_cpus < cpus) {
>           error_setg(errp, "maxcpus must be equal to or greater than smp");
> -        return;
> +        return true;
>       }
>   
>       if (sockets * cores * threads != ms->smp.max_cpus) {
> @@ -794,13 +794,15 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>                      "!= maxcpus (%u)",
>                      sockets, cores, threads,
>                      ms->smp.max_cpus);
> -        return;
> +        return true;
>       }
>   
>       ms->smp.cpus = cpus;
>       ms->smp.cores = cores;
>       ms->smp.threads = threads;
>       ms->smp.sockets = sockets;
> +
> +    return false;
>   }
>   
>   static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c2b9d62a358..84138a8bfd2 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -712,7 +712,7 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>    * This function is very similar to smp_parse()
>    * in hw/core/machine.c but includes CPU die support.
>    */
> -static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
> +static bool pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp)
>   {
>       unsigned cpus    = config->has_cpus ? config->cpus : 0;
>       unsigned sockets = config->has_sockets ? config->sockets : 0;
> @@ -743,14 +743,14 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>                      "sockets (%u) * dies (%u) * cores (%u) * threads (%u) < "
>                      "smp_cpus (%u)",
>                      sockets, dies, cores, threads, cpus);
> -        return;
> +        return true;
>       }
>   
>       ms->smp.max_cpus = config->has_maxcpus ? config->maxcpus : cpus;
>   
>       if (ms->smp.max_cpus < cpus) {
>           error_setg(errp, "maxcpus must be equal to or greater than smp");
> -        return;
> +        return true;
>       }
>   
>       if (sockets * dies * cores * threads != ms->smp.max_cpus) {
> @@ -759,7 +759,7 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>                      "!= maxcpus (%u)",
>                      sockets, dies, cores, threads,
>                      ms->smp.max_cpus);
> -        return;
> +        return true;
>       }
>   
>       ms->smp.cpus = cpus;
> @@ -767,6 +767,8 @@ static void pc_smp_parse(MachineState *ms, SMPConfiguration *config, Error **err
>       ms->smp.threads = threads;
>       ms->smp.sockets = sockets;
>       ms->smp.dies = dies;
> +
> +    return false;
>   }
>   
>   static
>