Re: [PULL 12/12] accel: abort if we fail to load the accelerator plugin

[Claudio Fontana <cfontana@suse.de>]

Hi all,

and thanks Paolo for taking care of this series, I think I noticed something that seems off:

On 11/6/22 09:51, Paolo Bonzini wrote:
> From: Claudio Fontana <cfontana@suse.de>
> 
> if QEMU is configured with modules enabled, it is possible that the
> load of an accelerator module will fail.
> Exit in this case, relying on module_object_class_by_name to report
> the specific load error if any.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> [claudio: changed abort() to exit(1)]
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Message-Id: <20220929093035.4231-6-cfontana@suse.de>
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/accel-softmmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> index 67276e4f5222..f9cdafb148ac 100644
> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -66,6 +66,7 @@ void accel_init_ops_interfaces(AccelClass *ac)
>  {
>      const char *ac_name;
>      char *ops_name;
> +    ObjectClass *oc;
>      AccelOpsClass *ops;
>  
>      ac_name = object_class_get_name(OBJECT_CLASS(ac));
> @@ -73,8 +74,13 @@ void accel_init_ops_interfaces(AccelClass *ac)
>  
>      ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
>      ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));

I think this last line should be removed. I somehow left it there by mistake.
Not sure if it hurts, but we assign to "ops" here,

only to reassign to the same variable...

I 

> +    oc = module_object_class_by_name(ops_name);
> +    if (!oc) {
> +        error_report("fatal: could not load module for type '%s'", ops_name);
> +        exit(1);
> +    }
>      g_free(ops_name);
> -
> +    ops = ACCEL_OPS_CLASS(oc);

here. Do I see it right?

>      /*
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.

So I suggest:

-     ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));                                                                          

Thanks and sorry for the oversight,

Claudio