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

[Claudio Fontana]

On 7/23/21 3:50 PM, Jose R. Ziviani wrote:
> On Fri, Jul 23, 2021 at 11:41:19AM +0200, Claudio Fontana wrote:
>> On 7/23/21 12:09 AM, Jose R. Ziviani wrote:
>>> 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 ...
>>>
>>> [2] qemu-system-x86_64 -accel tcg
>>> 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 ++++-
>>>  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);
>>> +    }
>>> +
>>
>>
>> Ah, again, why?
>> This change looks wrong to me, 
>>
>> the ops code should be present when ops interfaces are initialized:
>> it should be a code level assertion, as it has to do with the proper order 
>> of initializations in QEMU,
>>
>> why would we want to do anything else but to assert here?
>>
>> Am I blind to something obvious?
> 
> Hello!
> 
> Thank you for reviewing it!
> 
> The problem is that if your TCG module is not installed and you start
> QEMU like:
> 
> ./qemu-system-x86_64 -accel tcg
> 
> You'll get the error message + a crash with a core dump:
> 
> accel-tcg-x86_64 module is missing, install the package or config the library 
> path correctly.
> **
> 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]    5740 IOT instruction (core dumped)  ./qemu-system-x86_64 -accel tcg
> 
> I was digging a little bit more in order to move this responsibility to
> module.c but there isn't enough information there to safely exit() in
> all situations that a module may be loaded. As Gerd mentioned, more work
> is needed in order to achieve that.
> 
> However, it's not nice to have a crash due to an optional module missing.
> It's specially confusing because TCG has always been native. Considering
> also that we're already in hard freeze for 6.1, I thought to have this
> simpler check instead.
> 
> What do you think if we have something like:
> 
> /* FIXME: this isn't the right place to handle a missing module and
>    must be reverted when the module refactoring is completely done */
> #ifdef CONFIG_MODULES
> if (ops == NULL) {
>     exit(1);
> }
> #else
> g_assert(ops != NULL);
> #endif
> 
> Regards!


For the normal builds (without modular tcg), this issue does not appear right?
So maybe there is no pressure to change anything for 6.1, and we can work on 
the right solution on master?

Not sure how we consider this feature for 6.1, I guess it is still not a 
supported option,
(is there any CI for this? Probably not right?),

so I would consider building modular tcg in 6.1 as "experimental", and we can 
proceed to do the right thing on master?

Thanks,

Claudio

> 
>>
>>>      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
>>>  
>>>  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);
>>>      }
>>>  
>>> @@ -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
>>>
>>