Re: [Qemu-block] [PATCH for-2.8 v3 2/3] module: Don't load the same module if requested multiple times

[Max Reitz]

On 17.08.2016 09:20, Fam Zheng wrote:
> Use a hash table to keep record of all loaded modules, and return early
> if the requested module is already loaded.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  util/module.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/util/module.c b/util/module.c
> index a5f7fbd..63efad6 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -163,14 +163,29 @@ void module_load_one(const char *prefix, const char 
> *lib_name)
>      char *fname = NULL;
>      char *exec_dir;
>      char *dirs[3];
> +    char *module_name;
>      int i = 0;
>      int ret;
> +    static GHashTable *loaded_modules;
>  
>      if (!g_module_supported()) {
>          fprintf(stderr, "Module is not supported by system.\n");
>          return;
>      }
>  
> +    if (!loaded_modules) {
> +        loaded_modules = g_hash_table_new(g_str_hash, g_str_equal);
> +    }
> +
> +    module_name = g_strdup_printf("%s%s", prefix, lib_name);
> +
> +    if (g_hash_table_lookup(loaded_modules, module_name)) {
> +        fprintf(stderr, "module is already loaded: %s\n", module_name);

I'm not quite happy with this warning message. Loading a module is
automatically initiated by internal code in qemu, i.e. never done by the
user. Therefore, printing a message for the user does not make much
sense to me since the user cannot do anything about this.

If it is truly wrong to attempt to load a module more than once, this
should be an assertion.

However, I think it's perfectly fine to just allow qemu code to try to
load a module more than once and just ignore the request if we've
already loaded the module (as the commit message implies). In this case,
we don't need an error message or warning, though.

Max

> +        g_free(module_name);
> +        return;
> +    }
> +    g_hash_table_insert(loaded_modules, module_name, module_name);
> +
>      exec_dir = qemu_get_exec_dir();
>      dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
>      dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : "");
> @@ -180,8 +195,8 @@ void module_load_one(const char *prefix, const char 
> *lib_name)
>      exec_dir = NULL;
>  
>      for (i = 0; i < ARRAY_SIZE(dirs); i++) {
> -        fname = g_strdup_printf("%s/%s%s%s",
> -                dirs[i], prefix, lib_name, HOST_DSOSUF);
> +        fname = g_strdup_printf("%s/%s%s",
> +                dirs[i], module_name, HOST_DSOSUF);
>          ret = module_load_file(fname);
>          g_free(fname);
>          fname = NULL;
>

signature.asc
Description: OpenPGP digital signature