[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/1] modules: Improve error message when module is not fou
From: |
Jose R. Ziviani |
Subject: |
Re: [PATCH v2 1/1] modules: Improve error message when module is not found |
Date: |
Fri, 23 Jul 2021 12:46:25 -0300 |
On Fri, Jul 23, 2021 at 05:27:25PM +0200, Claudio Fontana wrote:
> On 7/23/21 4:36 PM, Jose R. Ziviani wrote:
> > On Fri, Jul 23, 2021 at 04:02:26PM +0200, 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?
> >
> > Yes, but OpenSUSE already builds with --enable-modules, we've already been
> > shipping
> > several modules as optional RPMs, like qemu-hw-display-virtio-gpu for
> > example. I sent
> > a patch some weeks ago to add "--enable-tcg-builtin" in the build system
> > but there're
> > more work required in that area as well.
> >
> >> 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?
> >
> > For OpenSUSE Tumbleweed, when we release QEMU 6.1, I can add my patch to
> > "--enable-tcg-builtin" for downstream only. I'm fine with it too.
>
> Hi Jose,
>
> indeed if we need to do something downstream it's fine,
> but lets keep the discussion on this list focused on what is best for
> upstream.
Absolutely! I'm sorry. I just wanted to answer that --enable-modules is
already used. Didn't mean cause any disturbance, sorry.
Thank you!!
>
> Ciao, thanks :-)
>
> Claudio
>
> >
> > Thank you!!!
> >
> >>
> >> 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
> >>>>>
> >>>>
> >>
>
signature.asc
Description: Digital signature
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, (continued)
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, Jose R. Ziviani, 2021/07/23
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, Claudio Fontana, 2021/07/23
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, Philippe Mathieu-Daudé, 2021/07/23
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, Jose R. Ziviani, 2021/07/23
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found, Claudio Fontana, 2021/07/23
- Re: [PATCH v2 1/1] modules: Improve error message when module is not found,
Jose R. Ziviani <=