Re: [PULL 37/40] machine: add smp compound property

[Peter Maydell]

On Tue, 6 Jul 2021 at 11:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Make -smp syntactic sugar for a compound property "-machine
> smp.{cores,threads,cpu,...}".  machine_smp_parse is replaced by the
> setter for the property.
>
> numa-test will now cover the new syntax, while other tests
> still use -smp.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Hi; Coverity reports a bug in this function (CID 1458083):

> +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();

ERRP_GUARD() ensures that 'errp' is never NULL...

> +
> +    if (!visit_type_SMPConfiguration(v, name, &config, errp)) {
> +        return;
> +    }
> +
> +    mc->smp_parse(ms, config, errp);
> +    if (errp) {
> +        goto out_free;

...so this condition is always true, and we always take the goto...

> +    }

...and the sanity-checks below here are dead code.

> +
> +    /* 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",
> +                   current_machine->smp.max_cpus,
> +                   mc->name, mc->max_cpus);
> +    }
> +
> +out_free:
> +    qapi_free_SMPConfiguration(config);
> +}

With ERRP_GUARD(), the test you want for "is there an error?"
is "if (*errp) {..."

thanks
-- PMM