Re: [Qemu-devel] [PATCH v12 5/8] module: implement module loading

[Fam Zheng]

On Thu, 10/10 14:16, Paolo Bonzini wrote:
> Il 10/10/2013 13:26, Fam Zheng ha scritto:
> > This patch adds loading, stamp checking and initialization of modules.
> > 
> > The init function of dynamic module is no longer directly called as
> > __attribute__((constructor)) in static linked version, it is called
> > only after passed the checking of presense of stamp symbol:
> > 
> >     qemu_stamp_$(date +%s$$$RANDOM)
> > 
> > With this, modules built from a different tree/version/configure will
> > not be loaded.
> > 
> > The module loading code requires gmodule-2.0.
> > 
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  Makefile              |   3 ++
> >  configure             |  32 ++++++++++-----
> >  include/qemu/module.h |  12 ++++++
> >  module-common.c       |  10 +++++
> >  rules.mak             |   7 ++--
> >  scripts/create_config |  14 +++++++
> >  util/module.c         | 107 
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  7 files changed, 170 insertions(+), 15 deletions(-)
> >  create mode 100644 module-common.c
> > 
> > diff --git a/Makefile b/Makefile
> > index a8488d6..51de298 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -196,6 +196,9 @@ Makefile: $(version-obj-y) $(version-lobj-y)
> >  libqemustub.a: $(stub-obj-y)
> >  libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
> >  
> > +block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) 
> > NULL
> > +util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
> > +
> >  ######################################################################
> >  
> >  qemu-img.o: qemu-img-cmds.h
> > diff --git a/configure b/configure
> > index 7b8771a..dd901ab 100755
> > --- a/configure
> > +++ b/configure
> > @@ -199,6 +199,7 @@ datadir="\${prefix}/share"
> >  qemu_docdir="\${prefix}/share/doc/qemu"
> >  bindir="\${prefix}/bin"
> >  libdir="\${prefix}/lib"
> > +moddir="\${prefix}/lib/qemu"
> >  libexecdir="\${prefix}/libexec"
> >  includedir="\${prefix}/include"
> >  sysconfdir="\${prefix}/etc"
> > @@ -660,7 +661,8 @@ for opt do
> >    ;;
> >    --disable-debug-info)
> >    ;;
> > -  --enable-modules) modules="yes"
> > +  --enable-modules)
> > +      modules="yes"
> >    ;;
> >    --cpu=*)
> >    ;;
> > @@ -685,6 +687,8 @@ for opt do
> >    ;;
> >    --libdir=*) libdir="$optarg"
> >    ;;
> > +  --moddir=*) moddir="$optarg"
> > +  ;;
> >    --libexecdir=*) libexecdir="$optarg"
> >    ;;
> >    --includedir=*) includedir="$optarg"
> > @@ -1084,6 +1088,7 @@ echo "  --datadir=PATH           install firmware in 
> > PATH$confsuffix"
> >  echo "  --docdir=PATH            install documentation in PATH$confsuffix"
> >  echo "  --bindir=PATH            install binaries in PATH"
> >  echo "  --libdir=PATH            install libraries in PATH"
> > +echo "  --moddir=PATH            install modules in PATH"
> 
> Is moddir needed?  It should always be LIBDIR/qemu.
> 

Hmm, basically an analogue to bindir and libdir.

I'm not sure why wasn't that libdir/bindir always be ${prefix}/lib and
${prefix}/bin as well. So why are they needed?

AFAICT it is not a problem to drop it, if you insist.

Fam

> Paolo
> 
> >  echo "  --sysconfdir=PATH        install config in PATH$confsuffix"
> >  echo "  --localstatedir=PATH     install local state in PATH (set at 
> > runtime on win32)"
> >  echo "  --with-confsuffix=SUFFIX suffix for QEMU data inside datadir and 
> > sysconfdir [$confsuffix]"
> > @@ -2291,15 +2296,19 @@ if test "$mingw32" = yes; then
> >  else
> >      glib_req_ver=2.12
> >  fi
> > -if $pkg_config --atleast-version=$glib_req_ver gthread-2.0; then
> > -    glib_cflags=`$pkg_config --cflags gthread-2.0`
> > -    glib_libs=`$pkg_config --libs gthread-2.0`
> > -    CFLAGS="$glib_cflags $CFLAGS"
> > -    LIBS="$glib_libs $LIBS"
> > -    libs_qga="$glib_libs $libs_qga"
> > -else
> > -    error_exit "glib-$glib_req_ver required to compile QEMU"
> > -fi
> > +
> > +for i in gthread-2.0 gmodule-2.0; do
> > +    if $pkg_config --atleast-version=$glib_req_ver $i; then
> > +        glib_cflags=`$pkg_config --cflags $i`
> > +        glib_libs=`$pkg_config --libs $i`
> > +        CFLAGS="$glib_cflags $CFLAGS"
> > +        LIBS="$glib_libs $LIBS"
> > +        libs_qga="$glib_libs $libs_qga"
> > +    else
> > +        error_exit "glib-$glib_req_ver required to compile QEMU"
> > +    fi
> > +done
> > +
> >  
> >  ##########################################
> >  # pixman support probe
> > @@ -3660,6 +3669,7 @@ echo "Install prefix    $prefix"
> >  echo "BIOS directory    `eval echo $qemu_datadir`"
> >  echo "binary directory  `eval echo $bindir`"
> >  echo "library directory `eval echo $libdir`"
> > +echo "module directory  `eval echo $moddir`"
> >  echo "libexec directory `eval echo $libexecdir`"
> >  echo "include directory `eval echo $includedir`"
> >  echo "config directory  `eval echo $sysconfdir`"
> > @@ -3786,6 +3796,7 @@ echo all: >> $config_host_mak
> >  echo "prefix=$prefix" >> $config_host_mak
> >  echo "bindir=$bindir" >> $config_host_mak
> >  echo "libdir=$libdir" >> $config_host_mak
> > +echo "moddir=$moddir" >> $config_host_mak
> >  echo "libexecdir=$libexecdir" >> $config_host_mak
> >  echo "includedir=$includedir" >> $config_host_mak
> >  echo "mandir=$mandir" >> $config_host_mak
> > @@ -3804,6 +3815,7 @@ echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
> >  
> >  echo "ARCH=$ARCH" >> $config_host_mak
> >  
> > +echo "CONFIG_STAMP=$(date +%s$$$RANDOM)" >> $config_host_mak
> >  if test "$modules" = "yes"; then
> >    echo "CONFIG_MODULES=y" >> $config_host_mak
> >  fi
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index c4ccd57..47b7f1d 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -14,11 +14,22 @@
> >  #ifndef QEMU_MODULE_H
> >  #define QEMU_MODULE_H
> >  
> > +#ifdef BUILD_DSO
> > +void DSO_STAMP_FUN(void);
> > +/* For error message, this function is an identification of qemu module */
> > +void qemu_module_dummy(void);
> > +
> > +#define module_init(function, type)                                        
> >  \
> > +static void __attribute__((constructor)) do_qemu_init_ ## function(void) { 
> >  \
> > +    register_dso_module_init(function, type);                              
> >  \
> > +}
> > +#else
> >  /* This should not be used directly.  Use block_init etc. instead.  */
> >  #define module_init(function, type)                                        
> >  \
> >  static void __attribute__((constructor)) do_qemu_init_ ## function(void) { 
> >  \
> >      register_module_init(function, type);                                  
> >  \
> >  }
> > +#endif
> >  
> >  typedef enum {
> >      MODULE_INIT_BLOCK,
> > @@ -34,6 +45,7 @@ typedef enum {
> >  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> >  
> >  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);
> >  
> > diff --git a/module-common.c b/module-common.c
> > new file mode 100644
> > index 0000000..50c6750
> > --- /dev/null
> > +++ b/module-common.c
> > @@ -0,0 +1,10 @@
> > +#include "config-host.h"
> > +#include "qemu/module.h"
> > +
> > +void qemu_module_dummy(void)
> > +{
> > +}
> > +
> > +void DSO_STAMP_FUN(void)
> > +{
> > +}
> > diff --git a/rules.mak b/rules.mak
> > index 2ff2f16..b0ccfcd 100644
> > --- a/rules.mak
> > +++ b/rules.mak
> > @@ -69,9 +69,9 @@ endif
> >  %.o: %.dtrace
> >     $(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   $(TARGET_DIR)$@")
> >  
> > -%$(DSOSUF): QEMU_CFLAGS += -fPIC
> > +%$(DSOSUF): QEMU_CFLAGS += -fPIC -DBUILD_DSO
> >  %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED)
> > -%$(DSOSUF): %.mo libqemustub.a
> > +%$(DSOSUF): %.mo libqemustub.a module-common.o
> >     $(call LINK,$^)
> >  
> >  .PHONY: modules
> > @@ -178,7 +178,8 @@ $(foreach o,$(filter %.o,$($1)),
> >     $(eval $(patsubst %.o,%.mo,$o)-objs := $o))
> >  $(foreach o,$(filter %.mo,$($1)),$(eval \
> >      $o: $($o-objs)))
> > -$(eval modules-m += $(patsubst %.o,%.mo,$($1)))
> > +$(eval t := $(patsubst %.o,%.mo,$($1)))
> > +$(foreach o,$t,$(eval modules-m += $o)))
> >  endef
> >  
> >  define unnest-vars
> > diff --git a/scripts/create_config b/scripts/create_config
> > index b1adbf5..d7ba61d 100755
> > --- a/scripts/create_config
> > +++ b/scripts/create_config
> > @@ -26,6 +26,17 @@ case $line in
> >      # save for the next definitions
> >      prefix=${line#*=}
> >      ;;
> > + moddir=*)
> > +    eval "moddir=\"${line#*=}\""
> > +    echo "#define CONFIG_MODDIR \"$moddir\""
> > +    ;;
> > + CONFIG_STAMP=*)
> > +    echo "#define DSO_STAMP_FUN qemu_stamp_${line#*=}"
> > +    echo "#define DSO_STAMP_FUN_STR \"qemu_stamp_${line#*=}\""
> > +    ;;
> > + CONFIG_MODULES=*)
> > +    echo "#define CONFIG_MODULES \"${line#*=}\""
> > +    ;;
> >   CONFIG_AUDIO_DRIVERS=*)
> >      drivers=${line#*=}
> >      echo "#define CONFIG_AUDIO_DRIVERS \\"
> > @@ -104,6 +115,9 @@ case $line in
> >      value=${line#*=}
> >      echo "#define $name $value"
> >      ;;
> > + DSOSUF=*)
> > +    echo "#define HOST_DSOSUF \"${line#*=}\""
> > +    ;;
> >  esac
> >  
> >  done # read
> > diff --git a/util/module.c b/util/module.c
> > index 7acc33d..c4115be 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -13,6 +13,7 @@
> >   * GNU GPL, version 2 or (at your option) any later version.
> >   */
> >  
> > +#include <gmodule.h>
> >  #include "qemu-common.h"
> >  #include "qemu/queue.h"
> >  #include "qemu/module.h"
> > @@ -21,13 +22,16 @@ typedef struct ModuleEntry
> >  {
> >      void (*init)(void);
> >      QTAILQ_ENTRY(ModuleEntry) node;
> > +    module_init_type type;
> >  } ModuleEntry;
> >  
> >  typedef QTAILQ_HEAD(, ModuleEntry) ModuleTypeList;
> >  
> >  static ModuleTypeList init_type_list[MODULE_INIT_MAX];
> >  
> > -static void init_types(void)
> > +static ModuleTypeList dso_init_list;
> > +
> > +static void init_lists(void)
> >  {
> >      static int inited;
> >      int i;
> > @@ -40,6 +44,8 @@ static void init_types(void)
> >          QTAILQ_INIT(&init_type_list[i]);
> >      }
> >  
> > +    QTAILQ_INIT(&dso_init_list);
> > +
> >      inited = 1;
> >  }
> >  
> > @@ -48,7 +54,7 @@ static ModuleTypeList *find_type(module_init_type type)
> >  {
> >      ModuleTypeList *l;
> >  
> > -    init_types();
> > +    init_lists();
> >  
> >      l = &init_type_list[type];
> >  
> > @@ -62,20 +68,117 @@ void register_module_init(void (*fn)(void), 
> > module_init_type type)
> >  
> >      e = g_malloc0(sizeof(*e));
> >      e->init = fn;
> > +    e->type = type;
> >  
> >      l = find_type(type);
> >  
> >      QTAILQ_INSERT_TAIL(l, e, node);
> >  }
> >  
> > +void register_dso_module_init(void (*fn)(void), module_init_type type)
> > +{
> > +    ModuleEntry *e;
> > +
> > +    init_lists();
> > +
> > +    e = g_malloc0(sizeof(*e));
> > +    e->init = fn;
> > +    e->type = type;
> > +
> > +    QTAILQ_INSERT_TAIL(&dso_init_list, e, node);
> > +}
> > +
> > +static void module_load(module_init_type type);
> > +
> >  void module_call_init(module_init_type type)
> >  {
> >      ModuleTypeList *l;
> >      ModuleEntry *e;
> >  
> > +    module_load(type);
> >      l = find_type(type);
> >  
> >      QTAILQ_FOREACH(e, l, node) {
> >          e->init();
> >      }
> >  }
> > +
> > +#ifdef CONFIG_MODULES
> > +static void module_load_file(const char *fname)
> > +{
> > +    GModule *g_module;
> > +    void (*sym)(void);
> > +    const char *dsosuf = HOST_DSOSUF;
> > +    int len = strlen(fname);
> > +    int suf_len = strlen(dsosuf);
> > +    ModuleEntry *e, *next;
> > +
> > +    if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
> > +        /* wrong suffix */
> > +        return;
> > +    }
> > +    if (access(fname, F_OK)) {
> > +        return;
> > +    }
> > +
> > +    assert(QTAILQ_EMPTY(&dso_init_list));
> > +
> > +    g_module = g_module_open(fname, G_MODULE_BIND_LAZY | 
> > G_MODULE_BIND_LOCAL);
> > +    if (!g_module) {
> > +        fprintf(stderr, "Failed to load module: %s\n",
> > +                g_module_error());
> > +        return;
> > +    }
> > +    if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
> > +        fprintf(stderr, "Failed to initialize module: %s\n",
> > +                fname);
> > +        /* Print some info if this is a QEMU module (but from different 
> > build),
> > +         * this will make debugging user problems easier. */
> > +        if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer 
> > *)&sym)) {
> > +            fprintf(stderr,
> > +                    "Note: only modules from the same build can be 
> > loaded.\n");
> > +        }
> > +        g_module_close(g_module);
> > +    } else {
> > +        QTAILQ_FOREACH(e, &dso_init_list, node) {
> > +            register_module_init(e->init, e->type);
> > +        }
> > +    }
> > +
> > +    QTAILQ_FOREACH_SAFE(e, &dso_init_list, node, next) {
> > +        QTAILQ_REMOVE(&dso_init_list, e, node);
> > +        g_free(e);
> > +    }
> > +}
> > +#endif
> > +
> > +void module_load(module_init_type type)
> > +{
> > +#ifdef CONFIG_MODULES
> > +    char *fname = NULL;
> > +    const char **mp;
> > +    static const char *block_modules[] = {
> > +        CONFIG_BLOCK_MODULES
> > +    };
> > +
> > +    if (!g_module_supported()) {
> > +        return;
> > +    }
> > +
> > +    switch (type) {
> > +    case MODULE_INIT_BLOCK:
> > +        mp = block_modules;
> > +        break;
> > +    /* no other types have dynamic modules for now*/
> > +    default:
> > +        return;
> > +    }
> > +
> > +    for ( ; *mp; mp++) {
> > +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, 
> > HOST_DSOSUF);
> > +        module_load_file(fname);
> > +        g_free(fname);
> > +    }
> > +
> > +#endif
> > +}
> > 
>