[PATCH] accel: print an error message and exit if plugin not loaded

[PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

If module_load_one, module_load_file fail for any reason
(permissions, plugin not installed, ...), we need to provide some notification
to the user to understand that this is happening; otherwise the errors
reported on initialization will make no sense to the user.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 accel/accel-softmmu.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..807708ee86 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -66,15 +66,21 @@ 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));
     g_assert(ac_name != NULL);
 
     ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
-    ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
+    oc = module_object_class_by_name(ops_name);
+    if (!oc) {
+        error_report("fatal: could not find module object of type \"%s\", "
+                     "plugin might not be loaded correctly", ops_name);
+        exit(EXIT_FAILURE);
+    }
     g_free(ops_name);
-
+    ops = ACCEL_OPS_CLASS(oc);
     /*
      * all accelerators need to define ops, providing at least a mandatory
      * non-NULL create_vcpu_thread operation.
-- 
2.26.2

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Richard Henderson]

On 9/5/22 11:13, Claudio Fontana wrote:
> If module_load_one, module_load_file fail for any reason
> (permissions, plugin not installed, ...), we need to provide some notification
> to the user to understand that this is happening; otherwise the errors
> reported on initialization will make no sense to the user.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>   accel/accel-softmmu.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> index 67276e4f52..807708ee86 100644
> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -66,15 +66,21 @@ 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));
>       g_assert(ac_name != NULL);
>   
>       ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
> -    ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
> +    oc = module_object_class_by_name(ops_name);
> +    if (!oc) {
> +        error_report("fatal: could not find module object of type \"%s\", "
> +                     "plugin might not be loaded correctly", ops_name);
> +        exit(EXIT_FAILURE);
> +    }

The change is correct, in that we certainly cannot continue without the accelerator loaded.

But I'm very disappointed that the module interface does not use Error, so you have no 
choice but to use an extremely vague message here.  I would much prefer to plumb down an 
error parameter so that here one could simply pass &error_fatal.


r~

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

On 9/5/22 14:06, Richard Henderson wrote:
> On 9/5/22 11:13, Claudio Fontana wrote:
>> If module_load_one, module_load_file fail for any reason
>> (permissions, plugin not installed, ...), we need to provide some notification
>> to the user to understand that this is happening; otherwise the errors
>> reported on initialization will make no sense to the user.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>   accel/accel-softmmu.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>> index 67276e4f52..807708ee86 100644
>> --- a/accel/accel-softmmu.c
>> +++ b/accel/accel-softmmu.c
>> @@ -66,15 +66,21 @@ 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));
>>       g_assert(ac_name != NULL);
>>   
>>       ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
>> -    ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
>> +    oc = module_object_class_by_name(ops_name);
>> +    if (!oc) {
>> +        error_report("fatal: could not find module object of type \"%s\", "
>> +                     "plugin might not be loaded correctly", ops_name);
>> +        exit(EXIT_FAILURE);
>> +    }
> 
> The change is correct, in that we certainly cannot continue without the accelerator loaded.
> 
> But I'm very disappointed that the module interface does not use Error, so you have no 
> choice but to use an extremely vague message here.  I would much prefer to plumb down an 
> error parameter so that here one could simply pass &error_fatal.
> 
> 
> r~
> 

I agree. I see it as also connected to:

https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00578.html

module_load_file actually has the pertinent information of what it going wrong at the time it goes wrong, so I presume we should collect the Error there,
and find a way not to lose the return value along the way..

Claudio

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

On 9/5/22 16:36, Claudio Fontana wrote:
> On 9/5/22 14:06, Richard Henderson wrote:
>> On 9/5/22 11:13, Claudio Fontana wrote:
>>> If module_load_one, module_load_file fail for any reason
>>> (permissions, plugin not installed, ...), we need to provide some notification
>>> to the user to understand that this is happening; otherwise the errors
>>> reported on initialization will make no sense to the user.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>   accel/accel-softmmu.c | 10 ++++++++--
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>>> index 67276e4f52..807708ee86 100644
>>> --- a/accel/accel-softmmu.c
>>> +++ b/accel/accel-softmmu.c
>>> @@ -66,15 +66,21 @@ 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));
>>>       g_assert(ac_name != NULL);
>>>   
>>>       ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
>>> -    ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
>>> +    oc = module_object_class_by_name(ops_name);
>>> +    if (!oc) {
>>> +        error_report("fatal: could not find module object of type \"%s\", "
>>> +                     "plugin might not be loaded correctly", ops_name);
>>> +        exit(EXIT_FAILURE);
>>> +    }
>>
>> The change is correct, in that we certainly cannot continue without the accelerator loaded.
>>
>> But I'm very disappointed that the module interface does not use Error, so you have no 
>> choice but to use an extremely vague message here.  I would much prefer to plumb down an 
>> error parameter so that here one could simply pass &error_fatal.
>>
>>
>> r~
>>
> 
> I agree. I see it as also connected to:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00578.html
> 
> module_load_file actually has the pertinent information of what it going wrong at the time it goes wrong, so I presume we should collect the Error there,
> and find a way not to lose the return value along the way..
> 
> Claudio
> 

Currently module_load_qom_one() is called among other things inside qom/object.c::object_initialize() as well.

Curiously enough, module_load_one(), which is in turn called by it, takes an argument "bool mayfail", which is always false,
never passed as true in the whole codebase:

bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);

/* mayfail is always false */

module_load_one calls in turn module_load_file, which also takes a bool mayfail argument:

static int module_load_file(const char *fname, bool mayfail, bool export_symbols);

You might think 'mayfail' can be called by other code as true in some cases, but no, it's always false.
I wonder why this "mayfail" argument exists and is propagated at all, when it cannot be anything else than false.
I tried to remove the "mayfail" parameter completely and things seem just fine.

In any case, the only thing that "mayfail" seems to control, is in module_load_file, and is a single printf:

    g_module = g_module_open(fname, flags);
    if (!g_module) {
        if (!mayfail) {
            fprintf(stderr, "Failed to open module: %s\n",
                    g_module_error());
        }
        ret = -EINVAL;
        goto out;
    }


Weird.. Is someone building proprietary modules on top of QEMU? Is this what this is currently trying to address?
But then, the result is just a printf...

Thanks,

C

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Richard Henderson]

On 9/5/22 16:43, Claudio Fontana wrote:
> You might think 'mayfail' can be called by other code as true in some cases, but no, it's always false.
> I wonder why this "mayfail" argument exists and is propagated at all, when it cannot be anything else than false.
> I tried to remove the "mayfail" parameter completely and things seem just fine.
> 
> In any case, the only thing that "mayfail" seems to control, is in module_load_file, and is a single printf:
> 
>      g_module = g_module_open(fname, flags);
>      if (!g_module) {
>          if (!mayfail) {
>              fprintf(stderr, "Failed to open module: %s\n",
>                      g_module_error());
>          }
>          ret = -EINVAL;
>          goto out;
>      }
> 
> 
> Weird.. Is someone building proprietary modules on top of QEMU? Is this what this is currently trying to address?
> But then, the result is just a printf...

I thought it was for things like vga_interface_available, which probes for the vga 
modules, but then gracefully handles an error.

There's definitely something wrong with the plumbing here.


r~

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Gerd Hoffmann]

> In any case, the only thing that "mayfail" seems to control, is in module_load_file, and is a single printf:
> 
>     g_module = g_module_open(fname, flags);
>     if (!g_module) {
>         if (!mayfail) {
>             fprintf(stderr, "Failed to open module: %s\n",
>                     g_module_error());
>         }
>         ret = -EINVAL;
>         goto out;
>     }
> 
> 
> Weird.. Is someone building proprietary modules on top of QEMU?

Nope.

But modules have dependencies to stuff like pci bus, usb bus, vga which
might not be satisfied by some system emulators, and trying to load
those modules will fail then because of unresolved symbols.  If you drop
that 'make check' will log a pile of errors ...

Dropping mayfail and return an 'Error' instead makes sense, then it is
up to the caller to report or not report the failure.  When calling down
from module_load_qom_all() you might want ignore errors for the reasons
outlined above, in most other caes it probably makes sense to report
them.

take care,
  Gerd

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

On 9/6/22 11:53, Gerd Hoffmann wrote:
>> In any case, the only thing that "mayfail" seems to control, is in module_load_file, and is a single printf:
>>
>>     g_module = g_module_open(fname, flags);
>>     if (!g_module) {
>>         if (!mayfail) {
>>             fprintf(stderr, "Failed to open module: %s\n",
>>                     g_module_error());
>>         }
>>         ret = -EINVAL;
>>         goto out;
>>     }
>>
>>
>> Weird.. Is someone building proprietary modules on top of QEMU?
> 
> Nope.
> 
> But modules have dependencies to stuff like pci bus, usb bus, vga which
> might not be satisfied by some system emulators, and trying to load
> those modules will fail then because of unresolved symbols.  If you drop
> that 'make check' will log a pile of errors ...
> 
> Dropping mayfail and return an 'Error' instead makes sense, then it is
> up to the caller to report or not report the failure.  When calling down
> from module_load_qom_all() you might want ignore errors for the reasons
> outlined above, in most other caes it probably makes sense to report
> them.
> 
> take care,
>   Gerd
> 
> 

Ah I noticed only now... I just sent a series, the module_load_qom_all() then is maybe something to discuss further.

Thanks,

Claudio

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Claudio Fontana]

On 9/6/22 13:59, Claudio Fontana wrote:
> On 9/6/22 11:53, Gerd Hoffmann wrote:
>>> In any case, the only thing that "mayfail" seems to control, is in module_load_file, and is a single printf:
>>>
>>>     g_module = g_module_open(fname, flags);
>>>     if (!g_module) {
>>>         if (!mayfail) {
>>>             fprintf(stderr, "Failed to open module: %s\n",
>>>                     g_module_error());
>>>         }
>>>         ret = -EINVAL;
>>>         goto out;
>>>     }
>>>
>>>
>>> Weird.. Is someone building proprietary modules on top of QEMU?
>>
>> Nope.
>>
>> But modules have dependencies to stuff like pci bus, usb bus, vga which
>> might not be satisfied by some system emulators, and trying to load
>> those modules will fail then because of unresolved symbols.  If you drop
>> that 'make check' will log a pile of errors ...
>>
>> Dropping mayfail and return an 'Error' instead makes sense, then it is
>> up to the caller to report or not report the failure.  When calling down
>> from module_load_qom_all() you might want ignore errors for the reasons
>> outlined above, in most other caes it probably makes sense to report
>> them.
>>
>> take care,
>>   Gerd
>>
>>
> 
> Ah I noticed only now... I just sent a series, the module_load_qom_all() then is maybe something to discuss further.
> 
> Thanks,
> 
> Claudio
> 

I noticed however that module_load_qom_all() does _not_ pass true for mayfail.

You changed this behavior in:

commit 9f4a0f0978cde9d8e27453b3f2d3679b53623c47
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Thu Jun 24 12:38:17 2021 +0200

    modules: use modinfo for qom load
    
    Use module database to figure which module implements a given QOM type.
    Drop hard-coded object list.
    
    Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
    Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
    Reviewed-by: Jose R. Ziviani <jziviani@suse.de>
    Message-Id: <20210624103836.2382472-16-kraxel@redhat.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


and from the patch I understand that this made the mayfail argument completely unnecessary, is that correct?

Thanks,

Claudio

Re: [PATCH] accel: print an error message and exit if plugin not loaded

[Gerd Hoffmann]

> > Ah I noticed only now... I just sent a series, the module_load_qom_all() then is maybe something to discuss further.
> > 
> > Thanks,
> > 
> > Claudio
> > 
> 
> I noticed however that module_load_qom_all() does _not_ pass true for mayfail.
> 
> You changed this behavior in:
> 
> commit 9f4a0f0978cde9d8e27453b3f2d3679b53623c47
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Thu Jun 24 12:38:17 2021 +0200
> 
>     modules: use modinfo for qom load

Oh.  Not sure this was actually intentional or a cut+paste bug.  The
change looks unrelated.  See also the other reply discussion missing
modules + load_all() sent a few minutes ago.

take care,
  Gerd