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

[Markus Armbruster <armbru@redhat.com>]

"Jose R. Ziviani" <jziviani@suse.de> writes:

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

This isn't an error message, it's a crash :)

> [2] qemu-system-x86_64 -accel tcg
> accel-tcg-x86_64 module is missing, install the package or config the library path correctly.

s/config/configure/

Also drop the period.

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

Not your patch's fault: I'm not sure the comment makes sense.

>      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

Why do you need to mess with the ifdeffery?

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

Again, not your patch's fault: reporting to stderr with fprintf() is
almost always wrong.

When the thing we report is an error, we should use error_report() for
correct formatting.  Likewise, warn_report() for warnings, info_report()
for informational messages.

When the module load is triggered by a monitor command, we probably want
to report problems to the monitor.  error_report() & friends do the
right thing for HMP.  For QMP, you have to use the Error API, i.e. have
the function take an Error ** argument, which the callers propagate all
the way to the QMP core.

To fix this issue, we first need to decide what kind of message this is:
error, warning, something else.

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