Re: [PATCH for-6.2 v4 12/14] machine: Put all sanity-check in the generic SMP parser

[Pankaj Gupta]

> Put both sanity-check of the input SMP configuration and sanity-check
> of the output SMP configuration uniformly in the generic parser. Then
> machine_set_smp() will become cleaner, also all the invalid scenarios
> can be tested only by calling the parser.
>
> Signed-off-by: Yanan Wang <wangyanan55@huawei.com>
> ---
>  hw/core/machine.c | 63 +++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 443ae5aa1b..3dd6c6f81e 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -811,6 +811,20 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> *config, Error **errp)
>      unsigned threads = config->has_threads ? config->threads : 0;
>      unsigned maxcpus = config->has_maxcpus ? config->maxcpus : 0;
>
> +    /*
> +     * A specified topology parameter must be greater than zero,
> +     * explicit configuration like "cpus=0" is not allowed.
> +     */
> +    if ((config->has_cpus && config->cpus == 0) ||
> +        (config->has_sockets && config->sockets == 0) ||
> +        (config->has_dies && config->dies == 0) ||
> +        (config->has_cores && config->cores == 0) ||
> +        (config->has_threads && config->threads == 0) ||
> +        (config->has_maxcpus && config->maxcpus == 0)) {
> +        error_setg(errp, "CPU topology parameters must be greater than 
> zero");
> +        return;
> +    }
> +
>      /*
>       * If not supported by the machine, a topology parameter must be
>       * omitted or specified equal to 1.
> @@ -889,6 +903,22 @@ static void smp_parse(MachineState *ms, SMPConfiguration 
> *config, Error **errp)
>                     topo_msg, maxcpus, cpus);
>          return;
>      }
> +
> +    if (ms->smp.cpus < mc->min_cpus) {
> +        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
> +                   "supported by machine '%s' is %d",
> +                   ms->smp.cpus,
> +                   mc->name, mc->min_cpus);
> +        return;
> +    }
> +
> +    if (ms->smp.max_cpus > mc->max_cpus) {
> +        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
> +                   "supported by machine '%s' is %d",
> +                   ms->smp.max_cpus,
> +                   mc->name, mc->max_cpus);
> +        return;
> +    }
>  }
>
>  static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> @@ -911,7 +941,6 @@ static void machine_get_smp(Object *obj, Visitor *v, 
> const char *name,
>  static void machine_set_smp(Object *obj, Visitor *v, const char *name,
>                              void *opaque, Error **errp)
>  {
> -    MachineClass *mc = MACHINE_GET_CLASS(obj);
>      MachineState *ms = MACHINE(obj);
>      SMPConfiguration *config;
>      ERRP_GUARD();
> @@ -920,40 +949,10 @@ static void machine_set_smp(Object *obj, Visitor *v, 
> const char *name,
>          return;
>      }
>
> -    /*
> -     * The CPU topology parameters must be specified greater than zero or
> -     * just omitted, explicit configuration like "cpus=0" is not allowed.
> -     */
> -    if ((config->has_cpus && config->cpus == 0) ||
> -        (config->has_sockets && config->sockets == 0) ||
> -        (config->has_dies && config->dies == 0) ||
> -        (config->has_cores && config->cores == 0) ||
> -        (config->has_threads && config->threads == 0) ||
> -        (config->has_maxcpus && config->maxcpus == 0)) {
> -        error_setg(errp, "CPU topology parameters must be greater than 
> zero");
> -        goto out_free;
> -    }
> -
>      smp_parse(ms, config, errp);
>      if (errp) {
> -        goto out_free;
> +        qapi_free_SMPConfiguration(config);
>      }
> -
> -    /* sanity-check smp_cpus and max_cpus against mc */
> -    if (ms->smp.cpus < mc->min_cpus) {
> -        error_setg(errp, "Invalid SMP CPUs %d. The min CPUs "
> -                   "supported by machine '%s' is %d",
> -                   ms->smp.cpus,
> -                   mc->name, mc->min_cpus);
> -    } else if (ms->smp.max_cpus > mc->max_cpus) {
> -        error_setg(errp, "Invalid SMP CPUs %d. The max CPUs "
> -                   "supported by machine '%s' is %d",
> -                   ms->smp.max_cpus,
> -                   mc->name, mc->max_cpus);
> -    }
> -
> -out_free:
> -    qapi_free_SMPConfiguration(config);
>  }
>
>  static void machine_class_init(ObjectClass *oc, void *data)

Looks good.

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>