Re: [PATCH 2/2] qom: Improve error message in module_object_class_by_name()

[Claudio Fontana]

On 7/21/21 11:34 AM, Daniel P. Berrangé wrote:
> On Tue, Jul 20, 2021 at 07:31:20PM -0300, Jose R. Ziviani wrote:
>> module_object_class_by_name() calls module_load_qom_one if the object
>> is provided by a dynamically linked library. Such library might not be
>> available at this moment - for instance, it can be a package not yet
>> installed. Thus, instead of assert error messages, this patch outputs
>> more friendly messages.
>>
>> Current error messages:
>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>> ...
>> ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion 
>> failed: (ops != NULL)
>> Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: 
>> assertion failed: (ops != NULL)
>> [1]    31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
>>
>> New error message:
>> $ ./qemu-system-x86_64 -machine q35 -accel tcg -kernel /boot/vmlinuz
>> accel-tcg-x86_64 module is missing, install the package or config the 
>> library path correctly.
>>
>> Or with other modules, when possible:
>> $ ./qemu-system-x86_64 -machine q35 -accel kvm -kernel /boot/vmlinuz -vga 
>> qxl                                                                     ✹
>> hw-display-qxl module is missing, install the package or config the library 
>> path correctly.
>> qemu-system-x86_64: QXL VGA not available
>>
>> $ make check
>> ...
>> Running test qtest-x86_64/test-filter-mirror
>> Running test qtest-x86_64/endianness-test
>> accel-qtest-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> accel-qtest-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> accel-qtest-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> accel-qtest-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> accel-qtest-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> accel-tcg-x86_64 module is missing, install the package or config the 
>> library path correctly.
>> ...
>>
>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>> ---
>>  accel/accel-softmmu.c | 5 ++++-
>>  qom/object.c          | 9 +++++++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
>> index 67276e4f52..52449ac2d0 100644
>> --- a/accel/accel-softmmu.c
>> +++ b/accel/accel-softmmu.c
>> @@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
>>       * all accelerators need to define ops, providing at least a mandatory
>>       * non-NULL create_vcpu_thread operation.
>>       */
>> -    g_assert(ops != NULL);
>> +    if (ops == NULL) {
>> +        exit(1);
>> +    }
>> +
>>      if (ops->ops_init) {
>>          ops->ops_init(ops);
>>      }
>> diff --git a/qom/object.c b/qom/object.c
>> index 6a01d56546..3a170ea9df 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -10,6 +10,7 @@
>>   * See the COPYING file in the top-level directory.
>>   */
>>  
>> +#include "qemu/module.h"
>>  #include "qemu/osdep.h"
>>  #include "hw/qdev-core.h"
>>  #include "qapi/error.h"
>> @@ -1031,8 +1032,16 @@ ObjectClass *module_object_class_by_name(const char 
>> *typename)
>>      oc = object_class_by_name(typename);
>>  #ifdef CONFIG_MODULES
>>      if (!oc) {
>> +        const char *module_name = module_get_name_from_obj(typename);
>>          module_load_qom_one(typename);
>>          oc = object_class_by_name(typename);
>> +        if (!oc && module_name) {
>> +            if (!module_is_loaded(module_name)) {
>> +                fprintf(stderr, "%s module is missing, install the "
>> +                                "package or config the library path "
>> +                                "correctly.\n", module_name);
>> +            }
>> +        }
>>      }
> 
> Introducing a call to module_is_loaded is feels like it isn't really
> addressing the root cause.
> 
> The error scenario here exists because module_load_qom_one() can
> fail to load the requested module. We don't detect that failure
> because module_load_qom_one() is "void" for some reason. There
> are several possible causes of failure and missing file on disk
> is only one of them.
> 
> This general error message about "module_name" being missing is
> misleading when there are dependancies involved. "module_name"
> may well exist, but something it depends on might be missing.
> Or the modules might have been built from wrong QEMU and be
> unable to be dlopened despite existing.
> 
> I think we need to fix module_load_qom_one() so that it can
> actually report failure via an "Error **errp" parameter.

Sounds right to me.

> This will also entail fixing a number of methods it in turn
> calls.
> 
> 
> Regards,
> Daniel
> 

Thanks,

Claudio