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

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

[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>
---
 block.c                        |  8 ++++----
 bsd-user/main.c                |  2 +-
 include/block/block.h          |  4 ++--
 include/qemu/module.h          |  2 +-
 linux-user/main.c              |  2 +-
 qemu-img.c                     |  2 +-
 qemu-io.c                      |  2 +-
 qemu-nbd.c                     |  2 +-
 qga/main.c                     |  2 +-
 tests/check-qom-interface.c    |  2 +-
 tests/test-qdev-global-props.c |  2 +-
 tests/test-qmp-commands.c      |  2 +-
 util/module.c                  | 11 ++++++-----
 vl.c                           |  6 +++---
 14 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index cb21a5f..f819f6e 100644
--- a/block.c
+++ b/block.c
@@ -4586,15 +4586,15 @@ BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
     return &acb->common;
 }
 
-void bdrv_init(void)
+void bdrv_init(const char *argv0)
 {
-    module_call_init(MODULE_INIT_BLOCK);
+    module_call_init(MODULE_INIT_BLOCK, argv0);
 }
 
-void bdrv_init_with_whitelist(void)
+void bdrv_init_with_whitelist(const char *argv0)
 {
     use_bdrv_whitelist = 1;
-    bdrv_init();
+    bdrv_init(argv0);
 }
 
 void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs,
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/include/block/block.h b/include/block/block.h
index 963a61f..7305232 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -166,8 +166,8 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data);
 void bdrv_io_limits_enable(BlockDriverState *bs);
 void bdrv_io_limits_disable(BlockDriverState *bs);
 
-void bdrv_init(void);
-void bdrv_init_with_whitelist(void);
+void bdrv_init(const char *argv0);
+void bdrv_init_with_whitelist(const char *argv0);
 BlockDriver *bdrv_find_protocol(const char *filename,
                                 bool allow_protocol_prefix);
 BlockDriver *bdrv_find_format(const char *format_name);
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 0fcac4f..110fc51 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -67,6 +67,6 @@ typedef enum {
 void register_module_init(void (*fn)(void), module_init_type type);
 void register_dso_module_init(void (*fn)(void), module_init_type type);
 
-void module_call_init(module_init_type type);
+void module_call_init(module_init_type type, const char *argv0);
 
 #endif
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();
diff --git a/qemu-img.c b/qemu-img.c
index c989850..9a169e3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2721,7 +2721,7 @@ int main(int argc, char **argv)
     error_set_progname(argv[0]);
 
     qemu_init_main_loop();
-    bdrv_init();
+    bdrv_init(argv[0]);
     if (argc < 2)
         help();
     cmdname = argv[1];
diff --git a/qemu-io.c b/qemu-io.c
index 7f459d8..736329d 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -440,7 +440,7 @@ int main(int argc, char **argv)
     }
 
     qemu_init_main_loop();
-    bdrv_init();
+    bdrv_init(argv[0]);
 
     /* initialize commands */
     qemuio_add_command(&quit_cmd);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 136e8c9..edebfec 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -583,7 +583,7 @@ int main(int argc, char **argv)
     }
 
     qemu_init_main_loop();
-    bdrv_init();
+    bdrv_init(argv[0]);
     atexit(bdrv_close_all);
 
     if (fmt) {
diff --git a/qga/main.c b/qga/main.c
index c58b26a..b448f5a 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -949,7 +949,7 @@ int main(int argc, char **argv)
     GList *blacklist = NULL;
     GAState *s;
 
-    module_call_init(MODULE_INIT_QAPI);
+    module_call_init(MODULE_INIT_QAPI, argv[0]);
 
     init_dfl_pathnames();
     pid_filepath = dfl_pathnames.pidfile;
diff --git a/tests/check-qom-interface.c b/tests/check-qom-interface.c
index f06380e..2e1480b 100644
--- a/tests/check-qom-interface.c
+++ b/tests/check-qom-interface.c
@@ -92,7 +92,7 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    module_call_init(MODULE_INIT_QOM);
+    module_call_init(MODULE_INIT_QOM, argv0);
     type_register_static(&test_if_info);
     type_register_static(&direct_impl_info);
     type_register_static(&intermediate_impl_info);
diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index e4ad173..242f1a8 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -166,7 +166,7 @@ int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
 
-    module_call_init(MODULE_INIT_QOM);
+    module_call_init(MODULE_INIT_QOM, argv0);
     type_register_static(&static_prop_type);
     type_register_static(&dynamic_prop_type);
 
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 5a3e82a..f46c2aa 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -175,7 +175,7 @@ int main(int argc, char **argv)
     g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
     g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
 
-    module_call_init(MODULE_INIT_QAPI);
+    module_call_init(MODULE_INIT_QAPI, argv0);
     g_test_run();
 
     return 0;
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 ? : "");
diff --git a/vl.c b/vl.c
index 383be1b..a07904c 100644
--- a/vl.c
+++ b/vl.c
@@ -2891,7 +2891,7 @@ int main(int argc, char **argv, char **envp)
 #endif
     }
 
-    module_call_init(MODULE_INIT_QOM);
+    module_call_init(MODULE_INIT_QOM, argv[0]);
 
     qemu_add_opts(&qemu_drive_opts);
     qemu_add_drive_opts(&qemu_legacy_drive_opts);
@@ -2927,7 +2927,7 @@ int main(int argc, char **argv, char **envp)
     QLIST_INIT (&vm_change_state_head);
     os_setup_early_signal_handling();
 
-    module_call_init(MODULE_INIT_MACHINE);
+    module_call_init(MODULE_INIT_MACHINE, argv[0]);
     machine = find_default_machine();
     cpu_model = NULL;
     ram_size = 0;
@@ -2943,7 +2943,7 @@ int main(int argc, char **argv, char **envp)
     nb_numa_nodes = 0;
     nb_nics = 0;
 
-    bdrv_init_with_whitelist();
+    bdrv_init_with_whitelist(argv[0]);
 
     autostart = 1;
 
-- 
1.8.5.4

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

[Paolo Bonzini]

Il 08/02/2014 05:40, Fam Zheng ha scritto:
> 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>

Really? :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                        |  8 ++++----
>  bsd-user/main.c                |  2 +-
>  include/block/block.h          |  4 ++--
>  include/qemu/module.h          |  2 +-
>  linux-user/main.c              |  2 +-
>  qemu-img.c                     |  2 +-
>  qemu-io.c                      |  2 +-
>  qemu-nbd.c                     |  2 +-
>  qga/main.c                     |  2 +-
>  tests/check-qom-interface.c    |  2 +-
>  tests/test-qdev-global-props.c |  2 +-
>  tests/test-qmp-commands.c      |  2 +-
>  util/module.c                  | 11 ++++++-----
>  vl.c                           |  6 +++---
>  14 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/block.c b/block.c
> index cb21a5f..f819f6e 100644
> --- a/block.c
> +++ b/block.c
> @@ -4586,15 +4586,15 @@ BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
>      return &acb->common;
>  }
>
> -void bdrv_init(void)
> +void bdrv_init(const char *argv0)
>  {
> -    module_call_init(MODULE_INIT_BLOCK);
> +    module_call_init(MODULE_INIT_BLOCK, argv0);
>  }
>
> -void bdrv_init_with_whitelist(void)
> +void bdrv_init_with_whitelist(const char *argv0)
>  {
>      use_bdrv_whitelist = 1;
> -    bdrv_init();
> +    bdrv_init(argv0);
>  }
>
>  void *qemu_aio_get(const AIOCBInfo *aiocb_info, BlockDriverState *bs,
> 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/include/block/block.h b/include/block/block.h
> index 963a61f..7305232 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -166,8 +166,8 @@ void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>  void bdrv_io_limits_enable(BlockDriverState *bs);
>  void bdrv_io_limits_disable(BlockDriverState *bs);
>
> -void bdrv_init(void);
> -void bdrv_init_with_whitelist(void);
> +void bdrv_init(const char *argv0);
> +void bdrv_init_with_whitelist(const char *argv0);
>  BlockDriver *bdrv_find_protocol(const char *filename,
>                                  bool allow_protocol_prefix);
>  BlockDriver *bdrv_find_format(const char *format_name);
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 0fcac4f..110fc51 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -67,6 +67,6 @@ typedef enum {
>  void register_module_init(void (*fn)(void), module_init_type type);
>  void register_dso_module_init(void (*fn)(void), module_init_type type);
>
> -void module_call_init(module_init_type type);
> +void module_call_init(module_init_type type, const char *argv0);
>
>  #endif
> 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();
> diff --git a/qemu-img.c b/qemu-img.c
> index c989850..9a169e3 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2721,7 +2721,7 @@ int main(int argc, char **argv)
>      error_set_progname(argv[0]);
>
>      qemu_init_main_loop();
> -    bdrv_init();
> +    bdrv_init(argv[0]);
>      if (argc < 2)
>          help();
>      cmdname = argv[1];
> diff --git a/qemu-io.c b/qemu-io.c
> index 7f459d8..736329d 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -440,7 +440,7 @@ int main(int argc, char **argv)
>      }
>
>      qemu_init_main_loop();
> -    bdrv_init();
> +    bdrv_init(argv[0]);
>
>      /* initialize commands */
>      qemuio_add_command(&quit_cmd);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 136e8c9..edebfec 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -583,7 +583,7 @@ int main(int argc, char **argv)
>      }
>
>      qemu_init_main_loop();
> -    bdrv_init();
> +    bdrv_init(argv[0]);
>      atexit(bdrv_close_all);
>
>      if (fmt) {
> diff --git a/qga/main.c b/qga/main.c
> index c58b26a..b448f5a 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -949,7 +949,7 @@ int main(int argc, char **argv)
>      GList *blacklist = NULL;
>      GAState *s;
>
> -    module_call_init(MODULE_INIT_QAPI);
> +    module_call_init(MODULE_INIT_QAPI, argv[0]);
>
>      init_dfl_pathnames();
>      pid_filepath = dfl_pathnames.pidfile;
> diff --git a/tests/check-qom-interface.c b/tests/check-qom-interface.c
> index f06380e..2e1480b 100644
> --- a/tests/check-qom-interface.c
> +++ b/tests/check-qom-interface.c
> @@ -92,7 +92,7 @@ int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>
> -    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QOM, argv0);
>      type_register_static(&test_if_info);
>      type_register_static(&direct_impl_info);
>      type_register_static(&intermediate_impl_info);
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index e4ad173..242f1a8 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -166,7 +166,7 @@ int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>
> -    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QOM, argv0);
>      type_register_static(&static_prop_type);
>      type_register_static(&dynamic_prop_type);
>
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 5a3e82a..f46c2aa 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -175,7 +175,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/0.15/dealloc_types", test_dealloc_types);
>      g_test_add_func("/0.15/dealloc_partial", test_dealloc_partial);
>
> -    module_call_init(MODULE_INIT_QAPI);
> +    module_call_init(MODULE_INIT_QAPI, argv0);
>      g_test_run();
>
>      return 0;
> 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 ? : "");
> diff --git a/vl.c b/vl.c
> index 383be1b..a07904c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2891,7 +2891,7 @@ int main(int argc, char **argv, char **envp)
>  #endif
>      }
>
> -    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_QOM, argv[0]);
>
>      qemu_add_opts(&qemu_drive_opts);
>      qemu_add_drive_opts(&qemu_legacy_drive_opts);
> @@ -2927,7 +2927,7 @@ int main(int argc, char **argv, char **envp)
>      QLIST_INIT (&vm_change_state_head);
>      os_setup_early_signal_handling();
>
> -    module_call_init(MODULE_INIT_MACHINE);
> +    module_call_init(MODULE_INIT_MACHINE, argv[0]);
>      machine = find_default_machine();
>      cpu_model = NULL;
>      ram_size = 0;
> @@ -2943,7 +2943,7 @@ int main(int argc, char **argv, char **envp)
>      nb_numa_nodes = 0;
>      nb_nics = 0;
>
> -    bdrv_init_with_whitelist();
> +    bdrv_init_with_whitelist(argv[0]);
>
>      autostart = 1;
>
>

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

[Fam Zheng]

On Sat, Feb 8, 2014 at 10:12 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/02/2014 05:40, Fam Zheng ha scritto:
>
>> 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>
>
>
> Really? :)

As a credit, yes. ;)

Fam

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

[Andreas Färber]

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

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

[Alexander Graf]

On 08.02.2014, at 18:16, Andreas Färber <afaerber@suse.de> wrote:

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

argv[0] will still be the QEMU binary, but the wrapper sets the QEMU_ARV0 environment variable to the real one. So IIUC this change should be ok.


Alex

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

[Peter Maydell]

On 8 February 2014 04:40, Fam Zheng <famz@redhat.com> wrote:
> This adds parameter "argv0" in calling path from main() to
> module_call_init(). So that module loader knows the location of
> executable.

This patch looks kind of odd to me. Why are there so many
different places calling module_call_init() and passing in an
argv0? I would have expected that vl.c (and the equivalent main
functions for the tools) would just initialise the module system
once, passing in the argv0 at that point. It's not obvious why
the block layer should be handing argv0 around through bdrv_init
in order to (re-?) initialise modules.

linux-user and bsd-user should probably force "don't ever
build with loadable modules" if we don't do that already.

-- PMM

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

[Paolo Bonzini]

Il 08/02/2014 18:46, Peter Maydell ha scritto:
>> This adds parameter "argv0" in calling path from main() to
>> > module_call_init(). So that module loader knows the location of
>> > executable.
> This patch looks kind of odd to me. Why are there so many
> different places calling module_call_init() and passing in an
> argv0? I would have expected that vl.c (and the equivalent main
> functions for the tools) would just initialise the module system
> once, passing in the argv0 at that point.

I'm ambivalent about this.

> It's not obvious why
> the block layer should be handing argv0 around through bdrv_init
> in order to (re-?) initialise modules.

The executable directory is not found once and for all, it's recomputed 
on any call to module_load or os_find_datadir.

But I think this is pointless anyway.  The OS knows the executable file 
name, and the right thing to do is to extend support for finding the 
executable to all supported OSes.  It's a pity that glib doesn't have a 
function anyway.

Peter, does the patch using the Apple-specific function to find the 
executable work?

Paolo

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

[Peter Maydell]

On 8 February 2014 23:36, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/02/2014 18:46, Peter Maydell ha scritto:
>> It's not obvious why
>> the block layer should be handing argv0 around through bdrv_init
>> in order to (re-?) initialise modules.
>
> The executable directory is not found once and for all, it's recomputed on
> any call to module_load or os_find_datadir.

That's why we call os_find_datadir exactly once. That we're
doing the lookup on each call to module_load is kind of what
I'm suggesting is wrong...

> But I think this is pointless anyway.  The OS knows the executable file
> name, and the right thing to do is to extend support for finding the
> executable to all supported OSes.  It's a pity that glib doesn't have a
> function anyway.
>
> Peter, does the patch using the Apple-specific function to find the
> executable work?

Haven't checked it yet. I just don't really see what the point is
in having a huge amount of OS specific code to do something
which we already do in a portable way. It might be nice to abstract
out stashing initial-argv0 and adding a utility function for it.

If we do want to use OS-specific code, then we should be
consistent, ie change the datadir lookup to use it.

thanks
-- PMM

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

[Paolo Bonzini]

Il 09/02/2014 01:18, Peter Maydell ha scritto:
> Haven't checked it yet. I just don't really see what the point is
> in having a huge amount of OS specific code to do something
> which we already do in a portable way. It might be nice to abstract
> out stashing initial-argv0 and adding a utility function for it.
>
> If we do want to use OS-specific code, then we should be
> consistent, ie change the datadir lookup to use it.

It is using it already.  argv[0] is just a fallback, and Fam's patches 
moved the OS-specific code of os_find_datadir out of it so that module 
loading could reuse it.

I think there are cases where argv[0] cannot work, such as using the 
exec system call to invoke QEMU, and specifying a different argv[0] than 
the actually executed file; or invoking a non-installed QEMU executable 
by putting its directory in the PATH.  They are probably not happening 
in practice, but they are there.

Paolo

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

[Fam Zheng]

On Sun, Feb 9, 2014 at 2:46 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/02/2014 01:18, Peter Maydell ha scritto:
>
>> Haven't checked it yet. I just don't really see what the point is
>> in having a huge amount of OS specific code to do something
>> which we already do in a portable way. It might be nice to abstract
>> out stashing initial-argv0 and adding a utility function for it.
>>
>> If we do want to use OS-specific code, then we should be
>> consistent, ie change the datadir lookup to use it.
>
>
> It is using it already.  argv[0] is just a fallback, and Fam's patches moved
> the OS-specific code of os_find_datadir out of it so that module loading
> could reuse it.
>
> I think there are cases where argv[0] cannot work, such as using the exec
> system call to invoke QEMU, and specifying a different argv[0] than the
> actually executed file; or invoking a non-installed QEMU executable by
> putting its directory in the PATH.  They are probably not happening in
> practice, but they are there.
>

Searching for module files in paths relative to executable is mostly
used for convenience of developers. End users, ideally, should have
the modules installed under $libdir. So, do we care about the corner
cases of argv[0]?

Fam

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

[Fam Zheng]

On Sun, Feb 9, 2014 at 7:36 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/02/2014 18:46, Peter Maydell ha scritto:
>
>>> This adds parameter "argv0" in calling path from main() to
>>> > module_call_init(). So that module loader knows the location of
>>> > executable.
>>
>> This patch looks kind of odd to me. Why are there so many
>> different places calling module_call_init() and passing in an
>> argv0? I would have expected that vl.c (and the equivalent main
>> functions for the tools) would just initialise the module system
>> once, passing in the argv0 at that point.
>
>
> I'm ambivalent about this.
>
>
>> It's not obvious why
>> the block layer should be handing argv0 around through bdrv_init
>> in order to (re-?) initialise modules.
>
>
> The executable directory is not found once and for all, it's recomputed on
> any call to module_load or os_find_datadir.
>

How about compute it for once in main() and load in module_call_init()?

Fam

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

[Paolo Bonzini]

Il 09/02/2014 10:26, Fam Zheng ha scritto:
> > The executable directory is not found once and for all, it's recomputed on
> > any call to module_load or os_find_datadir.
> >
>
> How about compute it for once in main() and load in module_call_init()?

Sure, that's what Peter's suggesting.

Paolo

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

[Peter Maydell]

On 9 February 2014 06:46, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/02/2014 01:18, Peter Maydell ha scritto:
>
>> Haven't checked it yet. I just don't really see what the point is
>> in having a huge amount of OS specific code to do something
>> which we already do in a portable way. It might be nice to abstract
>> out stashing initial-argv0 and adding a utility function for it.
>>
>> If we do want to use OS-specific code, then we should be
>> consistent, ie change the datadir lookup to use it.
>
>
> It is using it already.  argv[0] is just a fallback, and Fam's patches moved
> the OS-specific code of os_find_datadir out of it so that module loading
> could reuse it.

Ah, sorry, I hadn't spotted that. OK, then I think we should use that
code (and I'll test the MacOS X version), but it should go in a
called-once-from main init function that stashes the answer in
a static variable, and then the 'get the path' function should just
read that. Then we won't have to pass argv0 around more than
we need to.

thanks
-- PMM

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

[Fam Zheng]

On Sun, Feb 9, 2014 at 7:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ah, sorry, I hadn't spotted that. OK, then I think we should use that
> code (and I'll test the MacOS X version), but it should go in a
> called-once-from main init function that stashes the answer in
> a static variable, and then the 'get the path' function should just
> read that. Then we won't have to pass argv0 around more than
> we need to.
>

OK, I'll do this. Thanks.

Fam