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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/23/21 4:02 PM, Claudio Fontana wrote:
> 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?

+1 as I don't see this feature ready.