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

[Fam Zheng <famz@redhat.com>]

On Thu, 09/12 15:08, Daniel P. Berrange wrote:
> On Thu, Sep 12, 2013 at 03:04:56PM +0800, Fam Zheng wrote:
> > Added three types of modules:
> > 
> >     typedef enum {
> >         MODULE_LOAD_BLOCK = 0,
> >         MODULE_LOAD_UI,
> >         MODULE_LOAD_NET,
> >         MODULE_LOAD_MAX,
> >     } module_load_type;
> > 
> > and their loading function:
> > 
> >     void module_load(module_load_type).
> > 
> > which loads all ".so" files in a subdir under "${PREFIX}/qemu/", e.g.
> > "/usr/lib/qemu/block". Modules of each type should be loaded before
> > respective subsystem initialization code.
> > 
> > Requires gmodule-2.0 from glib.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c               |  1 +
> >  configure             | 28 +++++++++++++++++---------
> >  include/qemu/module.h |  9 +++++++++
> >  scripts/create_config | 10 ++++++++++
> >  util/module.c         | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                  |  2 ++
> >  6 files changed, 96 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 26639e8..16ceaaf 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4008,6 +4008,7 @@ BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs,
> >  
> >  void bdrv_init(void)
> >  {
> > +    module_load(MODULE_LOAD_BLOCK);
> >      module_call_init(MODULE_INIT_BLOCK);
> >  }
> >  
> > diff --git a/configure b/configure
> > index 5bc7256..275b1a0 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"
> > @@ -676,6 +677,8 @@ for opt do
> >    ;;
> >    --libdir=*) libdir="$optarg"
> >    ;;
> > +  --moddir=*) moddir="$optarg"
> > +  ;;
> >    --libexecdir=*) libexecdir="$optarg"
> >    ;;
> >    --includedir=*) includedir="$optarg"
> > @@ -1052,6 +1055,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"
> >  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]"
> > @@ -2256,15 +2260,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
> > @@ -3557,6 +3565,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`"
> > @@ -3680,6 +3689,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
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index c4ccd57..f00bc25 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -37,4 +37,13 @@ void register_module_init(void (*fn)(void), module_init_type type);
> >  
> >  void module_call_init(module_init_type type);
> >  
> > +typedef enum {
> > +    MODULE_LOAD_BLOCK = 0,
> > +    MODULE_LOAD_UI,
> > +    MODULE_LOAD_NET,
> > +    MODULE_LOAD_MAX,
> > +} module_load_type;
> > +
> > +void module_load(module_load_type type);
> > +
> >  #endif
> > diff --git a/scripts/create_config b/scripts/create_config
> > index b1adbf5..50391c2 100755
> > --- a/scripts/create_config
> > +++ b/scripts/create_config
> > @@ -26,6 +26,13 @@ case $line in
> >      # save for the next definitions
> >      prefix=${line#*=}
> >      ;;
> > + moddir=*)
> > +    eval "moddir=\"${line#*=}\""
> > +    echo "#define CONFIG_MODDIR \"$moddir\""
> > +    ;;
> > + CONFIG_MODULES=*)
> > +    echo "#define CONFIG_MODULES \"${line#*=}\""
> > +    ;;
> >   CONFIG_AUDIO_DRIVERS=*)
> >      drivers=${line#*=}
> >      echo "#define CONFIG_AUDIO_DRIVERS \\"
> > @@ -104,6 +111,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..8dd3544 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -13,6 +13,8 @@
> >   * GNU GPL, version 2 or (at your option) any later version.
> >   */
> >  
> > +#include <gmodule.h>
> > +#include <dirent.h>
> >  #include "qemu-common.h"
> >  #include "qemu/queue.h"
> >  #include "qemu/module.h"
> > @@ -79,3 +81,56 @@ void module_call_init(module_init_type type)
> >          e->init();
> >      }
> >  }
> > +
> > +void module_load(module_load_type type)
> > +{
> > +#ifdef CONFIG_MODULES
> > +    const char *path;
> > +    const char *dsosuf = HOST_DSOSUF;
> > +    char *fname;
> > +    int suf_len = strlen(dsosuf);
> > +    DIR *dp;
> > +    struct dirent *ep = NULL;
> > +    GModule *g_module;
> > +
> > +    if (!g_module_supported()) {
> > +        return;
> > +    }
> > +
> > +    switch (type) {
> > +    case MODULE_LOAD_BLOCK:
> > +        path = CONFIG_MODDIR "/block/";
> > +        break;
> > +    case MODULE_LOAD_UI:
> > +        path = CONFIG_MODDIR "/ui/";
> > +        break;
> > +    case MODULE_LOAD_NET:
> > +        path = CONFIG_MODDIR "/net/";
> > +        break;
> > +    default:
> > +        return;
> > +    }
> > +
> > +    dp = opendir(path);
> > +    if (!dp) {
> > +        fprintf(stderr, "Failed to open dir %s\n", path);
> > +        return;
> > +    }
> > +    for (ep = readdir(dp); ep; ep = readdir(dp)) {
> 
> Per previous posting, I believe you should not be blindly loading
> all modules in the directory using readdir(). Should whitelist
> load only individual module files that are part of QEMU, to add
> a block against 3rd party out of tree modules.
> 
> In addition this still seems to lack any kind of symbol hash
> addition, which should be a blocker for merging this series.
> 
Yes, I'll add module list and symbol hash in next revision.

Thanks.

Fam