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

["Jose R. Ziviani" <jziviani@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 5177 bytes --]

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!

> 
> >      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
> > 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]