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 <cfont...@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
