Re: [PATCH v2 1/1] modules: Improve error message when module is not found

[Claudio Fontana]

Hello Markus

On 7/23/21 10:28 AM, Markus Armbruster wrote:
> "Jose R. Ziviani" <jziviani@suse.de> writes:
> 
>> When a module is not found, specially accelerators, QEMU displays
>> a error message that not easy to understand[1]. This patch improves
>> the readability by offering a user-friendly message[2].
>>
>> This patch also moves the accelerator ops check to runtine (instead
>> of the original g_assert) because it works better with dynamic
>> modules.
>>
>> [1] qemu-system-x86_64 -accel tcg
>> 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)
>>     31964 IOT instruction (core dumped)  ./qemu-system-x86_64 ...
> 
> This isn't an error message, it's a crash :)


Yes, and the reason of the crash is that the code that needs to initialize ops 
is missing.

The assert needs to _stay there_,
this is a symptom, not the cause of the problem,

we should not be blindly removing asserts.


> 
>> [2] qemu-system-x86_64 -accel tcg
>> accel-tcg-x86_64 module is missing, install the package or config the 
>> library path correctly.
> 
> s/config/configure/
> 
> Also drop the period.
> 
>>
>> Signed-off-by: Jose R. Ziviani <jziviani@suse.de>
>> ---
>>  accel/accel-softmmu.c |  5 ++++-
>>  util/module.c         | 14 ++++++++------
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> 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);
>> +    }
>> +
> 
> Not your patch's fault: I'm not sure the comment makes sense.


Yes, the comment makes sense.


> 
>>      if (ops->ops_init) {
>>          ops->ops_init(ops);
>>      }
>> diff --git a/util/module.c b/util/module.c
>> index 6bb4ad915a..268a8563fd 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool 
>> mayfail, bool export_symbols
>>  out:
>>      return ret;
>>  }
>> -#endif
> 
> Why do you need to mess with the ifdeffery?
> 
>>  
>>  bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>>  {
>>      bool success = false;
>> -
>> -#ifdef CONFIG_MODULES
>>      char *fname = NULL;
>>  #ifdef CONFIG_MODULE_UPGRADES
>>      char *version_dir;
>> @@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char 
>> *lib_name, bool mayfail)
>>  
>>      if (!success) {
>>          g_hash_table_remove(loaded_modules, module_name);
>> +        fprintf(stderr, "%s module is missing, install the "
>> +                        "package or config the library path "
>> +                        "correctly.\n", module_name);
>>          g_free(module_name);
>>      }
>>  
> 
> Again, not your patch's fault: reporting to stderr with fprintf() is
> almost always wrong.
> 
> When the thing we report is an error, we should use error_report() for
> correct formatting.  Likewise, warn_report() for warnings, info_report()
> for informational messages.
> 
> When the module load is triggered by a monitor command, we probably want
> to report problems to the monitor.  error_report() & friends do the
> right thing for HMP.  For QMP, you have to use the Error API, i.e. have
> the function take an Error ** argument, which the callers propagate all
> the way to the QMP core.
> 
> To fix this issue, we first need to decide what kind of message this is:
> error, warning, something else.
> 
>> @@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char 
>> *lib_name, bool mayfail)
>>          g_free(dirs[i]);
>>      }
>>  
>> -#endif
>>      return success;
>>  }
>>  
>> -#ifdef CONFIG_MODULES
>> -
>>  static bool module_loaded_qom_all;
>>  
>>  void module_load_qom_one(const char *type)
>> @@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
>>  void module_load_qom_one(const char *type) {}
>>  void module_load_qom_all(void) {}
>>  
>> +bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
>> +{
>> +    return false;
>> +}
>> +
>>  #endif
>