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

[Jose R. Ziviani]

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.

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