Re: [Qemu-devel] [PATCH v19 11/11] module: Pass argv[0] along the module load path

["Andreas Färber" <afaerber@suse.de>]

Am 08.02.2014 05:40, schrieb Fam Zheng:
> This adds parameter "argv0" in calling path from main() to
> module_call_init(). So that module loader knows the location of
> executable.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
[...]
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index f9246aa..2802d0c 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -749,7 +749,7 @@ int main(int argc, char **argv)
>      if (argc <= 1)
>          usage();
>  
> -    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QOM, argv[0]);
>  
>      if ((envlist = envlist_create()) == NULL) {
>          (void) fprintf(stderr, "Unable to allocate envlist\n");
[...]
> diff --git a/linux-user/main.c b/linux-user/main.c
> index cabc9e1..b01c0a9 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3805,7 +3805,7 @@ int main(int argc, char **argv, char **envp)
>      int ret;
>      int execfd;
>  
> -    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QOM, argv0);
>  
>      qemu_init_auxval(envp);
>      qemu_cache_utils_init();

Are you sure these two are going to do the expected thing? At least with
Alex' binfmt wrapper, argv[0] will be the path of the emulated binary
(e.g., /bin/ls) rather than QEMU's IIUC. CC'ing Alex and Riku.

My hope would be that it doesn't matter for *-user in that core QOM
classes being initialized here remain in the main executable, but
there's no code comment or statement in the commit message indicating
this corner case has actually been thought of in this otherwise trivial
refactoring.

[...]
> diff --git a/util/module.c b/util/module.c
> index c36b60a..e5bc30e 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -5,6 +5,7 @@
>   *
>   * Authors:
>   *  Anthony Liguori   <aliguori@us.ibm.com>
> + *  Fam Zheng <famz@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2.  See
>   * the COPYING file in the top-level directory.
> @@ -89,14 +90,14 @@ void register_dso_module_init(void (*fn)(void), module_init_type type)
>      QTAILQ_INSERT_TAIL(&dso_init_list, e, node);
>  }
>  
> -static void module_load(module_init_type type);
> +static void module_load(module_init_type type, const char *argv);
>  
> -void module_call_init(module_init_type type)
> +void module_call_init(module_init_type type, const char *argv)
>  {
>      ModuleTypeList *l;
>      ModuleEntry *e;
>  
> -    module_load(type);
> +    module_load(type, argv);
>      l = find_type(type);
>  
>      QTAILQ_FOREACH(e, l, node) {
> @@ -161,7 +162,7 @@ out:
>  }
>  #endif
>  
> -void module_load(module_init_type type)
> +void module_load(module_init_type type, const char *argv)
>  {
>  #ifdef CONFIG_MODULES
>      char *fname = NULL;
> @@ -188,7 +189,7 @@ void module_load(module_init_type type)
>          return;
>      }
>  
> -    exec_dir = qemu_exec_dir(NULL);
> +    exec_dir = qemu_exec_dir(argv);
>      dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
>      dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : "");
>      dirs[i++] = g_strdup_printf("%s", exec_dir ? : "");
[snip]

Suggest to consistently name them argv0 (like in the header) since
passing argv without [] to functions looks kind of odd. I assume the v
stands for vector, which a single argument isn't really (ignoring the
character vector).

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg