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