| Message ID | 20210813112608.1452541-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core: fix error checkig in smp_parse | expand |
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 >
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
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> --- include/hw/boards.h | 2 +- hw/core/machine.c | 12 +++++++----- hw/i386/pc.c | 10 ++++++---- 3 files changed, 14 insertions(+), 10 deletions(-)