[Qemu-devel] [PATCH v10 0/8] Shared Library Module Support

[Qemu-devel] [PATCH v10 0/8] Shared Library Module Support

[Fam Zheng]

This series implements feature of shared object building as described in:

http://wiki.qemu.org/Features/Modules

The main idea behind modules is to isolate dependencies on third party
libraries from qemu executables, such as libglusterfs or librbd, so that the
end users can install core qemu package with fewer dependencies.  And only for
those who want to use particular modules, need they install qemu-foo
sub-package, which in turn requires libbar and libbiz packages.

It's implemented in three steps:

1. The first patches fix current build system to correctly handle nested
   variables and object specific options:

    [01/08] ui/Makefile.objs: delete unnecessary cocoa.o dependency
    [02/08] make.rule: fix $(obj) to a real relative path
    [03/08] rule.mak: allow per object cflags and libs

2. The Makefile changes adds necessary options and rules to build DSO objects:

    [04/08] build-sys: introduce common-obj-m and block-obj-m for DSO

3. The next patch adds code to load modules from installed directory:

    [05/08] module: implement module loading

A few more changes are following to complete it:

    [06/08] Makefile: install modules with "make install"
    [07/08] .gitignore: ignore module related files (dll, so, mo)

In the end of series, the block drivers are converted:

    [08/08] block: convert block drivers linked with libs to modules

v10:
    All modules in a single directory (moddir), with module type prefixed:
        /usr/lib/qemu/block-{curl,iscsi,...}.so
    The module names for user to list in module whitelist is consequently:
        block-curl, block-iscsi, ui-*, etc.
    In Makfile, the installed module filename is simply generated by:
        $(subst /,-,%.so)
    Which is also the rule for module names.

    [05] Add #undef CONFIG_MODULE_WHITELIST in config-host.h.
         Use static array for whitelist. (Richard)

v9: Address Daniel's comment with patch 05:
    Drop readdir().
    Squash module whitelist changes to module loading patch.

v8: This version introduces two runtime loading checks:
    * Module init function no longer with __attribute__((constructor)), and
      mangled with per configure fingerprint. See patch 05.
    * Module whitelist as configure option.

    [04] Link libqemustub.a to DSO. (iscsi needs qemu_get_vm_name).
         Fix single object module link.
         Fix extract-libs to also extract .o libs that'd be expanded later from
         .mo.
    [05] Add init function name mangling.
    [06] New.
    [07] Fix typo in "make install". [Daniel]

Fam Zheng (7):
  make.rule: fix $(obj) to a real relative path
  rule.mak: allow per object cflags and libs
  build-sys: introduce common-obj-m and block-obj-m for DSO
  module: implement module loading
  Makefile: install modules with "make install"
  .gitignore: ignore module related files (dll, so, mo)
  block: convert block drivers linked with libs to modules

Peter Maydell (1):
  ui/Makefile.objs: delete unnecessary cocoa.o dependency

 .gitignore            |  3 ++
 Makefile              | 30 +++++++++++++++++-
 Makefile.objs         | 19 ++----------
 Makefile.target       | 21 ++++++++++---
 block.c               |  1 +
 block/Makefile.objs   | 11 ++++++-
 configure             | 86 +++++++++++++++++++++++++++++++++++----------------
 include/qemu/module.h | 23 ++++++++++++++
 rules.mak             | 84 +++++++++++++++++++++++++++++++++++++++++--------
 scripts/create_config | 22 +++++++++++++
 ui/Makefile.objs      |  2 --
 util/module.c         | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c                  |  2 ++
 13 files changed, 322 insertions(+), 63 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v10 1/8] ui/Makefile.objs: delete unnecessary cocoa.o dependency

[Fam Zheng]

From: Peter Maydell <peter.maydell@linaro.org>

Delete an unnecessary dependency for cocoa.o; we already have
a general rule that tells Make that we can build a .o file
from a .m source using an ObjC compiler, so this specific
rule is unnecessary. Further, it is using the dubious construct
"$(SRC_PATH)/$(obj)" to get at the source directory, which will
break when $(obj) is redefined as part of the preparation for
per-object library support.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 ui/Makefile.objs | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 6ddc0de..f33be47 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -17,6 +17,4 @@ common-obj-$(CONFIG_GTK) += gtk.o x_keymap.o
 
 $(obj)/sdl.o $(obj)/sdl_zoom.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
 
-$(obj)/cocoa.o: $(SRC_PATH)/$(obj)/cocoa.m
-
 $(obj)/gtk.o: QEMU_CFLAGS += $(GTK_CFLAGS) $(VTE_CFLAGS)
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 2/8] make.rule: fix $(obj) to a real relative path

[Fam Zheng]

Makefile.target includes rule.mak and unnested common-obj-y, then prefix
them with '../', this will ignore object specific QEMU_CFLAGS in subdir
Makefile.objs:

    $(obj)/curl.o: QEMU_CFLAGS += $(CURL_CFLAGS)

Because $(obj) here is './block', instead of '../block'. This doesn't
hurt compiling because we basically build all .o from top Makefile,
before entering Makefile.target, but it will affact arriving per-object
libs support.

The starting point of $(obj) is passed in as argument of unnest-vars, as
well as nested variables, so that different Makefiles can pass in a
right value.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile        | 14 ++++++++++++++
 Makefile.objs   | 17 +----------------
 Makefile.target | 17 +++++++++++++----
 configure       |  1 +
 rules.mak       | 14 +++++++++-----
 5 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/Makefile b/Makefile
index 362fe3e..b56579b 100644
--- a/Makefile
+++ b/Makefile
@@ -115,6 +115,16 @@ defconfig:
 
 ifneq ($(wildcard config-host.mak),)
 include $(SRC_PATH)/Makefile.objs
+endif
+
+dummy := $(call unnest-vars,, \
+                stub-obj-y \
+                util-obj-y \
+                qga-obj-y \
+                block-obj-y \
+                common-obj-y)
+
+ifneq ($(wildcard config-host.mak),)
 include $(SRC_PATH)/tests/Makefile
 endif
 ifeq ($(CONFIG_SMARTCARD_NSS),y)
@@ -123,6 +133,10 @@ endif
 
 all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all
 
+vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
+
+vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
+
 config-host.h: config-host.h-timestamp
 config-host.h-timestamp: config-host.mak
 qemu-options.def: $(SRC_PATH)/qemu-options.hx
diff --git a/Makefile.objs b/Makefile.objs
index 2b6c1fe..91235a6 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -41,7 +41,7 @@ libcacard-y += libcacard/vcardt.o
 # single QEMU executable should support all CPUs and machines.
 
 ifeq ($(CONFIG_SOFTMMU),y)
-common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o block/
+common-obj-y = blockdev.o blockdev-nbd.o block/
 common-obj-y += net/
 common-obj-y += readline.o
 common-obj-y += qdev-monitor.o device-hotplug.o
@@ -110,18 +110,3 @@ version-lobj-$(CONFIG_WIN32) += $(BUILD_DIR)/version.lo
 # by libqemuutil.a.  These should be moved to a separate .json schema.
 qga-obj-y = qga/ qapi-types.o qapi-visit.o
 qga-vss-dll-obj-y = qga/
-
-vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
-
-vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
-
-QEMU_CFLAGS+=$(GLIB_CFLAGS)
-
-nested-vars += \
-	stub-obj-y \
-	util-obj-y \
-	qga-obj-y \
-	qga-vss-dll-obj-y \
-	block-obj-y \
-	common-obj-y
-dummy := $(call unnest-vars)
diff --git a/Makefile.target b/Makefile.target
index 9a49852..87906ea 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -143,13 +143,22 @@ endif # CONFIG_SOFTMMU
 # Workaround for http://gcc.gnu.org/PR55489, see configure.
 %/translate.o: QEMU_CFLAGS += $(TRANSLATE_OPT_CFLAGS)
 
-nested-vars += obj-y
+dummy := $(call unnest-vars,,obj-y)
 
-# This resolves all nested paths, so it must come last
+# we are making another call to unnest-vars with different vars, protect obj-y,
+# it can be overriden in subdir Makefile.objs
+obj-y-save := $(obj-y)
+
+block-obj-y :=
+common-obj-y :=
 include $(SRC_PATH)/Makefile.objs
+dummy := $(call unnest-vars,..,block-obj-y common-obj-y)
+
+# Now restore obj-y
+obj-y := $(obj-y-save)
+
+all-obj-y = $(obj-y) $(common-obj-y) $(block-obj-y)
 
-all-obj-y = $(obj-y)
-all-obj-y += $(addprefix ../, $(common-obj-y))
 
 ifndef CONFIG_HAIKU
 LIBS+=-lm
diff --git a/configure b/configure
index 2b83936..6ac4b55 100755
--- a/configure
+++ b/configure
@@ -2279,6 +2279,7 @@ 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
diff --git a/rules.mak b/rules.mak
index abc2e84..01e552e 100644
--- a/rules.mak
+++ b/rules.mak
@@ -110,9 +110,6 @@ clean: clean-timestamp
 
 # magic to descend into other directories
 
-obj := .
-old-nested-dirs :=
-
 define push-var
 $(eval save-$2-$1 = $(value $1))
 $(eval $1 :=)
@@ -126,9 +123,11 @@ endef
 
 define unnest-dir
 $(foreach var,$(nested-vars),$(call push-var,$(var),$1/))
-$(eval obj := $(obj)/$1)
+$(eval obj-parent-$1 := $(obj))
+$(eval obj := $(if $(obj),$(obj)/$1,$1))
 $(eval include $(SRC_PATH)/$1/Makefile.objs)
-$(eval obj := $(patsubst %/$1,%,$(obj)))
+$(eval obj := $(obj-parent-$1))
+$(eval obj-parent-$1 := )
 $(foreach var,$(nested-vars),$(call pop-var,$(var),$1/))
 endef
 
@@ -143,7 +142,12 @@ $(if $(nested-dirs),
 endef
 
 define unnest-vars
+$(eval obj := $1)
+$(eval nested-vars := $2)
+$(eval old-nested-dirs := )
 $(call unnest-vars-1)
+$(if $1,$(foreach v,$(nested-vars),$(eval \
+	$v := $(addprefix $1/,$($v)))))
 $(foreach var,$(nested-vars),$(eval $(var) := $(filter-out %/, $($(var)))))
 $(shell mkdir -p $(sort $(foreach var,$(nested-vars),$(dir $($(var))))))
 $(foreach var,$(nested-vars), $(eval \
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 3/8] rule.mak: allow per object cflags and libs

[Fam Zheng]

Adds extract-libs in LINK to expand any "per object libs", the syntax to define
such a libs options is like:

        foo.o-libs := $(CURL_LIBS)

in block/Makefile.objs.

Similarly,

        foo.o-cflags := $(FOO_CFLAGS)

is also supported.

"foo.o" must be listed a nested var (e.g. common-obj-y) to make the
option variables effective.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 rules.mak | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/rules.mak b/rules.mak
index 01e552e..e732261 100644
--- a/rules.mak
+++ b/rules.mak
@@ -21,15 +21,17 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
 # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
 QEMU_INCLUDES += -I$(<D) -I$(@D)
 
+extract-libs = $(strip $(foreach o,$1,$($o-libs)))
+
 %.o: %.c
-	$(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) -c -o $@ $<,"  CC    $(TARGET_DIR)$@")
+	$(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) $($@-cflags) -c -o $@ $<,"  CC    $(TARGET_DIR)$@")
 %.o: %.rc
 	$(call quiet-command,$(WINDRES) -I. -o $@ $<,"  RC    $(TARGET_DIR)$@")
 
 ifeq ($(LIBTOOL),)
 LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
        $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
-       $(LIBS),"  LINK  $(TARGET_DIR)$@")
+       $(call extract-libs,$^) $(LIBS),"  LINK  $(TARGET_DIR)$@")
 else
 LIBTOOL += $(if $(V),,--quiet)
 %.lo: %.c
@@ -45,7 +47,7 @@ LINK = $(call quiet-command,\
        $(sort $(filter %.o, $1)) $(filter-out %.o, $1) \
        $(if $(filter %.lo %.la,$^),$(version-lobj-y),$(version-obj-y)) \
        $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
-       $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
+       $(call extract-libs,$^) $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
 endif
 
 %.asm: %.S
@@ -121,11 +123,22 @@ $(eval $1 = $(value save-$2-$1) $$(subdir-$2-$1))
 $(eval save-$2-$1 :=)
 endef
 
+define fix-obj-vars
+$(foreach v,$($1), \
+	$(if $($v-cflags), \
+		$(eval $2$v-cflags := $($v-cflags)) \
+		$(eval $v-cflags := )) \
+	$(if $($v-libs), \
+		$(eval $2$v-libs := $($v-libs)) \
+		$(eval $v-libs := )))
+endef
+
 define unnest-dir
 $(foreach var,$(nested-vars),$(call push-var,$(var),$1/))
 $(eval obj-parent-$1 := $(obj))
 $(eval obj := $(if $(obj),$(obj)/$1,$1))
 $(eval include $(SRC_PATH)/$1/Makefile.objs)
+$(foreach v,$(nested-vars),$(call fix-obj-vars,$v,$(if $(obj),$(obj)/)))
 $(eval obj := $(obj-parent-$1))
 $(eval obj-parent-$1 := )
 $(foreach var,$(nested-vars),$(call pop-var,$(var),$1/))
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 4/8] build-sys: introduce common-obj-m and block-obj-m for DSO

[Fam Zheng]

Add necessary rules and flags for shared object generation.
$(common-obj-m) will include $(block-obj-m), like $(common-obj-y) does
for $(block-obj-y). The new rules introduced here are:

0) For all %.so compiling:

    QEMU_CFLAGS += -fPIC

1) %.o in $(common-obj-m) is compiled to %.o, then linked to %.so.

2) %.mo in $(common-obj-m) is the placeholder for %.so for pattern
matching in Makefile. It's linked to "-shared" with all its dependencies
(multiple *.o) as input. Which means the list of depended objects must
be specified in each sub-Makefile.objs:

    foo.mo-objs := bar.o baz.o qux.o

in the same style with foo.o-cflags and foo.o-libs. The objects here
will be prefixed with "$(obj)/" if it's a subdirectory Makefile.objs.

Also introduce --enable-modules in configure, the option will enable
support of shared object build. Otherwise objects are static linked to
executables.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile        |  9 +++++++--
 Makefile.objs   |  2 ++
 Makefile.target |  6 +++++-
 configure       | 14 ++++++++++++++
 rules.mak       | 54 +++++++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index b56579b..b59b49c 100644
--- a/Makefile
+++ b/Makefile
@@ -122,7 +122,9 @@ dummy := $(call unnest-vars,, \
                 util-obj-y \
                 qga-obj-y \
                 block-obj-y \
-                common-obj-y)
+                block-obj-m \
+                common-obj-y \
+                common-obj-m)
 
 ifneq ($(wildcard config-host.mak),)
 include $(SRC_PATH)/tests/Makefile
@@ -131,7 +133,7 @@ ifeq ($(CONFIG_SMARTCARD_NSS),y)
 include $(SRC_PATH)/libcacard/Makefile
 endif
 
-all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all
+all: $(DOCS) $(TOOLS) $(HELPERS-y) recurse-all modules
 
 vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
@@ -249,6 +251,9 @@ clean:
 	rm -f qemu-options.def
 	find . -name '*.[oda]' -type f -exec rm -f {} +
 	find . -name '*.l[oa]' -type f -exec rm -f {} +
+	find . -name '*.so' -type f -exec rm -f {} +
+	find . -name '*.mo' -type f -exec rm -f {} +
+	find . -name '*.dll' -type f -exec rm -f {} +
 	rm -f $(filter-out %.tlb,$(TOOLS)) $(HELPERS-y) qemu-ga TAGS cscope.* *.pod *~ */*~
 	rm -Rf .libs
 	rm -f qemu-img-cmds.h
diff --git a/Makefile.objs b/Makefile.objs
index 91235a6..072d2e5 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -19,6 +19,8 @@ block-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
 block-obj-y += qemu-coroutine-sleep.o
 block-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
 
+block-obj-m = block/
+
 ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
 # Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
 # only pull in the actual virtio-9p device if we also enabled virtio.
diff --git a/Makefile.target b/Makefile.target
index 87906ea..7fb9e4d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -152,7 +152,11 @@ obj-y-save := $(obj-y)
 block-obj-y :=
 common-obj-y :=
 include $(SRC_PATH)/Makefile.objs
-dummy := $(call unnest-vars,..,block-obj-y common-obj-y)
+dummy := $(call unnest-vars,.., \
+               block-obj-y \
+               block-obj-m \
+               common-obj-y \
+               common-obj-m)
 
 # Now restore obj-y
 obj-y := $(obj-y-save)
diff --git a/configure b/configure
index 6ac4b55..55a75d8 100755
--- a/configure
+++ b/configure
@@ -190,6 +190,9 @@ mingw32="no"
 gcov="no"
 gcov_tool="gcov"
 EXESUF=""
+DSOSUF=".so"
+LDFLAGS_SHARED="-shared"
+modules="no"
 prefix="/usr/local"
 mandir="\${prefix}/share/man"
 datadir="\${prefix}/share"
@@ -496,6 +499,7 @@ OpenBSD)
 Darwin)
   bsd="yes"
   darwin="yes"
+  LDFLAGS_SHARED="-bundle"
   if [ "$cpu" = "x86_64" ] ; then
     QEMU_CFLAGS="-arch x86_64 $QEMU_CFLAGS"
     LDFLAGS="-arch x86_64 $LDFLAGS"
@@ -595,6 +599,7 @@ fi
 
 if test "$mingw32" = "yes" ; then
   EXESUF=".exe"
+  DSOSUF=".dll"
   QEMU_CFLAGS="-DWIN32_LEAN_AND_MEAN -DWINVER=0x501 $QEMU_CFLAGS"
   # enable C99/POSIX format strings (needs mingw32-runtime 3.15 or later)
   QEMU_CFLAGS="-D__USE_MINGW_ANSI_STDIO=1 $QEMU_CFLAGS"
@@ -659,6 +664,8 @@ for opt do
   ;;
   --disable-debug-info)
   ;;
+  --enable-modules) modules="yes"
+  ;;
   --cpu=*)
   ;;
   --target-list=*) target_list="$optarg"
@@ -1074,6 +1081,7 @@ echo "  --libdir=PATH            install libraries 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]"
+echo "  --enable-modules         enable modules support"
 echo "  --enable-debug-tcg       enable TCG debugging"
 echo "  --disable-debug-tcg      disable TCG debugging (default)"
 echo "  --enable-debug-info       enable debugging information (default)"
@@ -3660,6 +3668,7 @@ echo "python            $python"
 if test "$slirp" = "yes" ; then
     echo "smbd              $smbd"
 fi
+echo "module support    $modules"
 echo "host CPU          $cpu"
 echo "host big endian   $bigendian"
 echo "target list       $target_list"
@@ -3778,6 +3787,9 @@ echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
 
 echo "ARCH=$ARCH" >> $config_host_mak
 
+if test "$modules" = "yes"; then
+  echo "CONFIG_MODULES=y" >> $config_host_mak
+fi
 case "$cpu" in
   arm|i386|x86_64|x32|ppc|aarch64)
     # The TCG interpreter currently does not support ld/st optimization.
@@ -4269,6 +4281,8 @@ echo "LIBTOOLFLAGS=$LIBTOOLFLAGS" >> $config_host_mak
 echo "LIBS+=$LIBS" >> $config_host_mak
 echo "LIBS_TOOLS+=$libs_tools" >> $config_host_mak
 echo "EXESUF=$EXESUF" >> $config_host_mak
+echo "DSOSUF=$DSOSUF" >> $config_host_mak
+echo "LDFLAGS_SHARED=$LDFLAGS_SHARED" >> $config_host_mak
 echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
 echo "POD2MAN=$POD2MAN" >> $config_host_mak
 echo "TRANSLATE_OPT_CFLAGS=$TRANSLATE_OPT_CFLAGS" >> $config_host_mak
diff --git a/rules.mak b/rules.mak
index e732261..2ff2f16 100644
--- a/rules.mak
+++ b/rules.mak
@@ -21,7 +21,11 @@ QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
 # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
 QEMU_INCLUDES += -I$(<D) -I$(@D)
 
-extract-libs = $(strip $(foreach o,$1,$($o-libs)))
+extract-libs = $(strip $(sort $(foreach o,$1,$($o-libs)) \
+                  $(foreach o,$(call expand-objs,$1),$($o-libs))))
+expand-objs = $(strip $(sort $(filter %.o,$1)) \
+                  $(foreach o,$(filter %.mo,$1),$($o-objs)) \
+                  $(filter-out %.o %.mo,$1))
 
 %.o: %.c
 	$(call quiet-command,$(CC) $(QEMU_INCLUDES) $(QEMU_CFLAGS) $(QEMU_DGFLAGS) $(CFLAGS) $($@-cflags) -c -o $@ $<,"  CC    $(TARGET_DIR)$@")
@@ -30,8 +34,8 @@ extract-libs = $(strip $(foreach o,$1,$($o-libs)))
 
 ifeq ($(LIBTOOL),)
 LINK = $(call quiet-command,$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
-       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) $(version-obj-y) \
-       $(call extract-libs,$^) $(LIBS),"  LINK  $(TARGET_DIR)$@")
+       $(call expand-objs,$1) $(version-obj-y) \
+       $(call extract-libs,$1) $(LIBS),"  LINK  $(TARGET_DIR)$@")
 else
 LIBTOOL += $(if $(V),,--quiet)
 %.lo: %.c
@@ -42,12 +46,12 @@ LIBTOOL += $(if $(V),,--quiet)
 	$(call quiet-command,$(LIBTOOL) --mode=compile --tag=CC dtrace -o $@ -G -s $<, " lt GEN $(TARGET_DIR)$@")
 
 LINK = $(call quiet-command,\
-       $(if $(filter %.lo %.la,$^),$(LIBTOOL) --mode=link --tag=CC \
+       $(if $(filter %.lo %.la,$1),$(LIBTOOL) --mode=link --tag=CC \
        )$(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) -o $@ \
-       $(sort $(filter %.o, $1)) $(filter-out %.o, $1) \
-       $(if $(filter %.lo %.la,$^),$(version-lobj-y),$(version-obj-y)) \
-       $(if $(filter %.lo %.la,$^),$(LIBTOOLFLAGS)) \
-       $(call extract-libs,$^) $(LIBS),$(if $(filter %.lo %.la,$^),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
+       $(call expand-objs,$1) \
+       $(if $(filter %.lo %.la,$1),$(version-lobj-y),$(version-obj-y)) \
+       $(if $(filter %.lo %.la,$1),$(LIBTOOLFLAGS)) \
+       $(call extract-libs,$1) $(LIBS),$(if $(filter %.lo %.la,$1),"lt LINK ", "  LINK  ")"$(TARGET_DIR)$@")
 endif
 
 %.asm: %.S
@@ -65,6 +69,17 @@ endif
 %.o: %.dtrace
 	$(call quiet-command,dtrace -o $@ -G -s $<, "  GEN   $(TARGET_DIR)$@")
 
+%$(DSOSUF): QEMU_CFLAGS += -fPIC
+%$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED)
+%$(DSOSUF): %.mo libqemustub.a
+	$(call LINK,$^)
+
+.PHONY: modules
+modules:
+
+%.mo:
+	$(call quiet-command,touch $@,"  GEN   $(TARGET_DIR)$@")
+
 %$(EXESUF): %.o
 	$(call LINK,$^)
 
@@ -130,7 +145,10 @@ $(foreach v,$($1), \
 		$(eval $v-cflags := )) \
 	$(if $($v-libs), \
 		$(eval $2$v-libs := $($v-libs)) \
-		$(eval $v-libs := )))
+		$(eval $v-libs := )) \
+	$(if $($v-objs), \
+		$(eval $2$v-objs := $(addprefix $2,$($v-objs))) \
+		$(eval $v-objs := )))
 endef
 
 define unnest-dir
@@ -154,6 +172,15 @@ $(if $(nested-dirs),
   $(call unnest-vars-1))
 endef
 
+define add-modules
+$(foreach o,$(filter %.o,$($1)),
+	$(eval $(patsubst %.o,%.mo,$o): $o) \
+	$(eval $(patsubst %.o,%.mo,$o)-objs := $o))
+$(foreach o,$(filter %.mo,$($1)),$(eval \
+    $o: $($o-objs)))
+$(eval modules-m += $(patsubst %.o,%.mo,$($1)))
+endef
+
 define unnest-vars
 $(eval obj := $1)
 $(eval nested-vars := $2)
@@ -165,4 +192,13 @@ $(foreach var,$(nested-vars),$(eval $(var) := $(filter-out %/, $($(var)))))
 $(shell mkdir -p $(sort $(foreach var,$(nested-vars),$(dir $($(var))))))
 $(foreach var,$(nested-vars), $(eval \
   -include $(addsuffix *.d, $(sort $(dir $($(var)))))))
+$(foreach v,$(filter %-m,$(nested-vars)), \
+    $(call add-modules,$v))
+
+$(if $(CONFIG_MODULES), \
+    $(eval modules: $(patsubst %.mo,%$(DSOSUF),$(modules-m))), \
+    $(foreach v,$(filter %-m,$(nested-vars)), \
+        $(eval $(patsubst %-m,%-y,$v) += $($v)) \
+        $(eval $v := )))
+
 endef
-- 
1.8.3.1

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

[Fam Zheng]

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 whitelisted ".so" files of the given type under ${MODDIR}.

Modules of each type should be loaded in respective subsystem
initialization code.

The init function of dynamic module is no longer with
__attribute__((constructor)) as static linked version, and need to be
explicitly called once loaded. The function name is mangled with per
configure fingerprint as:

    init_$(date +%s$$$RANDOM)

Which is known to module_load function, and the loading fails if this
symbol is not there. With this, modules built from a different
tree/version/configure will not be loaded.

The module loading code requires gmodule-2.0.

Configure option "--enable-modules=L" can be used to restrict qemu to
only build/load some whitelisted modules.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile              |  3 ++
 block.c               |  1 +
 configure             | 44 +++++++++++++++++++++-------
 include/qemu/module.h | 23 +++++++++++++++
 rules.mak             |  9 ++++--
 scripts/create_config | 22 ++++++++++++++
 util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c                  |  2 ++
 8 files changed, 172 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index b59b49c..20167b8 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
 
+default-whitelist = $(foreach o,$(modules-m),"$(subst /,-,$o)",) NULL
+util/module.o-cflags = -D'CONFIG_MODULE_WHITELIST=$(default-whitelist)'
+
 ######################################################################
 
 qemu-img.o: qemu-img-cmds.h
diff --git a/block.c b/block.c
index a325efc..cd4f90e 100644
--- a/block.c
+++ b/block.c
@@ -3897,6 +3897,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 55a75d8..9cb9c7b 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"
@@ -664,7 +665,9 @@ for opt do
   ;;
   --disable-debug-info)
   ;;
-  --enable-modules) modules="yes"
+  --enable-modules|--enable-modules=*)
+      modules="yes"
+      module_list=`echo "$optarg" | sed -e 's/,/ /g'`
   ;;
   --cpu=*)
   ;;
@@ -689,6 +692,8 @@ for opt do
   ;;
   --libdir=*) libdir="$optarg"
   ;;
+  --moddir=*) moddir="$optarg"
+  ;;
   --libexecdir=*) libexecdir="$optarg"
   ;;
   --includedir=*) includedir="$optarg"
@@ -1078,10 +1083,14 @@ 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]"
-echo "  --enable-modules         enable modules support"
+echo "  --enable-modules         enable modules support and whitelist all modules"
+echo "  --enable-modules=L       enable modules and provide a whitelist"
+echo "                           Available modules: block-curl block-iscsi block-gluster"
+echo "                                              block-ssh block-rbd"
 echo "  --enable-debug-tcg       enable TCG debugging"
 echo "  --disable-debug-tcg      disable TCG debugging (default)"
 echo "  --enable-debug-info       enable debugging information (default)"
@@ -2284,15 +2293,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
@@ -3643,6 +3656,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`"
@@ -3669,6 +3683,9 @@ if test "$slirp" = "yes" ; then
     echo "smbd              $smbd"
 fi
 echo "module support    $modules"
+if test -n "$module_list"; then
+    echo "module whitelist  $module_list"
+fi
 echo "host CPU          $cpu"
 echo "host big endian   $bigendian"
 echo "target list       $target_list"
@@ -3769,6 +3786,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
@@ -3787,8 +3805,12 @@ echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
 
 echo "ARCH=$ARCH" >> $config_host_mak
 
+echo "CONFIG_FINGERPRINT=$(date +%s$$$RANDOM)" >> $config_host_mak
 if test "$modules" = "yes"; then
   echo "CONFIG_MODULES=y" >> $config_host_mak
+  if test -n "$module_list"; then
+      echo "CONFIG_MODULE_WHITELIST=$module_list" >> $config_host_mak
+  fi
 fi
 case "$cpu" in
   arm|i386|x86_64|x32|ppc|aarch64)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index c4ccd57..6458d8f 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -14,11 +14,25 @@
 #ifndef QEMU_MODULE_H
 #define QEMU_MODULE_H
 
+#ifdef BUILD_DSO
+
+/* For error message, this function is an identification of qemu module */
+void qemu_module_do_init(void (*init)(void));
+
+/* To restrict loading of arbitrary DSO, The init function name is changed per
+ * "./configure" to refuse unknown DSO file */
+void DSO_INIT_FUN(void);
+
+#define module_init(function, type)                                         \
+void qemu_module_do_init(void (*init)(void)) { init(); }                    \
+void DSO_INIT_FUN(void) { qemu_module_do_init(function); }
+#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,
@@ -37,4 +51,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/rules.mak b/rules.mak
index 2ff2f16..dc9757a 100644
--- a/rules.mak
+++ b/rules.mak
@@ -69,7 +69,7 @@ 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
 	$(call LINK,$^)
@@ -172,13 +172,18 @@ $(if $(nested-dirs),
   $(call unnest-vars-1))
 endef
 
+is-whitelisted = $(if $(CONFIG_MODULE_WHITELIST),$(strip \
+    $(filter $(CONFIG_MODULE_WHITELIST),$(subst /,-,$(basename $1)))),\
+	yes)
 define add-modules
 $(foreach o,$(filter %.o,$($1)),
 	$(eval $(patsubst %.o,%.mo,$o): $o) \
 	$(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,$(if $(call is-whitelisted,$o),$(eval \
+	modules-m += $o)))
 endef
 
 define unnest-vars
diff --git a/scripts/create_config b/scripts/create_config
index b1adbf5..6d47df7 100755
--- a/scripts/create_config
+++ b/scripts/create_config
@@ -26,6 +26,25 @@ case $line in
     # save for the next definitions
     prefix=${line#*=}
     ;;
+ moddir=*)
+    eval "moddir=\"${line#*=}\""
+    echo "#define CONFIG_MODDIR \"$moddir\""
+    ;;
+ CONFIG_FINGERPRINT=*)
+    echo "#define DSO_INIT_FUN init_${line#*=}"
+    echo "#define DSO_INIT_FUN_STR \"init_${line#*=}\""
+    ;;
+ CONFIG_MODULES=*)
+    echo "#define CONFIG_MODULES \"${line#*=}\""
+    ;;
+ CONFIG_MODULE_WHITELIST=*)
+    echo "#undef CONFIG_MODULE_WHITELIST"
+    echo "#define CONFIG_MODULE_WHITELIST\\"
+    for mod in ${line#*=}; do
+      echo "    \"${mod}\",\\"
+    done
+    echo "    NULL"
+    ;;
  CONFIG_AUDIO_DRIVERS=*)
     drivers=${line#*=}
     echo "#define CONFIG_AUDIO_DRIVERS \\"
@@ -104,6 +123,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..70da1ca 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"
@@ -79,3 +80,83 @@ void module_call_init(module_init_type type)
         e->init();
     }
 }
+
+#ifdef CONFIG_MODULES
+static void module_load_file(const char *fname)
+{
+    GModule *g_module;
+    void (*init_fun)(void);
+    const char *dsosuf = HOST_DSOSUF;
+    int len = strlen(fname);
+    int suf_len = strlen(dsosuf);
+
+    if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
+        /* wrong suffix */
+        return;
+    }
+    if (access(fname, F_OK)) {
+        return;
+    }
+
+    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_INIT_FUN_STR, (gpointer *)&init_fun)) {
+        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_do_init",
+                            (gpointer *)&init_fun)) {
+            fprintf(stderr,
+                    "Note: only modules from the same build can be loaded.\n");
+        }
+        g_module_close(g_module);
+        return;
+    }
+    init_fun();
+}
+#endif
+
+void module_load(module_load_type type)
+{
+#ifdef CONFIG_MODULES
+    const char *prefix;
+    char *fname = NULL;
+    const char **mp;
+    static const char *module_whitelist[] = {
+        CONFIG_MODULE_WHITELIST
+    };
+
+    if (!g_module_supported()) {
+        return;
+    }
+
+    switch (type) {
+    case MODULE_LOAD_BLOCK:
+        prefix = "block-";
+        break;
+    case MODULE_LOAD_UI:
+        prefix = "ui-";
+        break;
+    case MODULE_LOAD_NET:
+        prefix = "ui-";
+        break;
+    default:
+        return;
+    }
+
+    for (mp = &module_whitelist[0]; *mp; mp++) {
+        if (strncmp(prefix, *mp, strlen(prefix))) {
+            continue;
+        }
+        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
+        module_load_file(fname);
+        g_free(fname);
+    }
+
+#endif
+}
diff --git a/vl.c b/vl.c
index 4e709d5..b20e7fb 100644
--- a/vl.c
+++ b/vl.c
@@ -2866,6 +2866,8 @@ int main(int argc, char **argv, char **envp)
 #endif
     }
 
+    module_load(MODULE_LOAD_UI);
+    module_load(MODULE_LOAD_NET);
     module_call_init(MODULE_INIT_QOM);
 
     qemu_add_opts(&qemu_drive_opts);
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 6/8] Makefile: install modules with "make install"

[Fam Zheng]

Install all the modules to ${MODDIR}.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile
index 20167b8..a254f1c 100644
--- a/Makefile
+++ b/Makefile
@@ -363,6 +363,12 @@ install-datadir install-localstatedir
 ifneq ($(TOOLS),)
 	$(INSTALL_PROG) $(STRIP_OPT) $(TOOLS) "$(DESTDIR)$(bindir)"
 endif
+ifneq ($(CONFIG_MODULES),)
+	$(INSTALL_DIR) "$(DESTDIR)$(moddir)"
+	for s in $(patsubst %.mo,%$(DSOSUF),$(modules-m)); do \
+		$(INSTALL_PROG) $(STRIP_OPT) $$s "$(DESTDIR)$(moddir)/$${s//\//-}"; \
+	done
+endif
 ifneq ($(HELPERS-y),)
 	$(INSTALL_DIR) "$(DESTDIR)$(libexecdir)"
 	$(INSTALL_PROG) $(STRIP_OPT) $(HELPERS-y) "$(DESTDIR)$(libexecdir)"
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 7/8] .gitignore: ignore module related files (dll, so, mo)

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 .gitignore | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/.gitignore b/.gitignore
index 8e1b73f..ac679ea 100644
--- a/.gitignore
+++ b/.gitignore
@@ -63,6 +63,9 @@ fsdev/virtfs-proxy-helper.pod
 *.cp
 *.dvi
 *.exe
+*.dll
+*.so
+*.mo
 *.fn
 *.ky
 *.log
-- 
1.8.3.1

[Qemu-devel] [PATCH v10 8/8] block: convert block drivers linked with libs to modules

[Fam Zheng]

The converted block drivers are:

    curl
    iscsi
    rbd
    ssh
    glusterfs

no longer adds flags and libs for them to global variables, instead
create config-host.mak variables like FOO_CFLAGS and FOO_LIBS, which is
used as per object cflags and libs.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/Makefile.objs | 11 ++++++++++-
 configure           | 33 +++++++++++++++------------------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 3bb85b5..f98d379 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -23,4 +23,13 @@ common-obj-y += commit.o
 common-obj-y += mirror.o
 common-obj-y += backup.o
 
-$(obj)/curl.o: QEMU_CFLAGS+=$(CURL_CFLAGS)
+iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
+iscsi.o-libs       := $(LIBISCSI_LIBS)
+curl.o-cflags      := $(CURL_CFLAGS)
+curl.o-libs        := $(CURL_LIBS)
+rbd.o-cflags       := $(RBD_CFLAGS)
+rbd.o-libs         := $(RBD_LIBS)
+gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
+gluster.o-libs     := $(GLUSTERFS_LIBS)
+ssh.o-cflags       := $(LIBSSH2_CFLAGS)
+ssh.o-libs         := $(LIBSSH2_LIBS)
diff --git a/configure b/configure
index 9cb9c7b..cbb8f42 100755
--- a/configure
+++ b/configure
@@ -2254,8 +2254,6 @@ EOF
   curl_libs=`$curlconfig --libs 2>/dev/null`
   if compile_prog "$curl_cflags" "$curl_libs" ; then
     curl=yes
-    libs_tools="$curl_libs $libs_tools"
-    libs_softmmu="$curl_libs $libs_softmmu"
   else
     if test "$curl" = "yes" ; then
       feature_not_found "curl"
@@ -2415,8 +2413,6 @@ EOF
   rbd_libs="-lrbd -lrados"
   if compile_prog "" "$rbd_libs" ; then
     rbd=yes
-    libs_tools="$rbd_libs $libs_tools"
-    libs_softmmu="$rbd_libs $libs_softmmu"
   else
     if test "$rbd" = "yes" ; then
       feature_not_found "rados block device"
@@ -2433,9 +2429,6 @@ if test "$libssh2" != "no" ; then
     libssh2_cflags=`$pkg_config libssh2 --cflags`
     libssh2_libs=`$pkg_config libssh2 --libs`
     libssh2=yes
-    libs_tools="$libssh2_libs $libs_tools"
-    libs_softmmu="$libssh2_libs $libs_softmmu"
-    QEMU_CFLAGS="$QEMU_CFLAGS $libssh2_cflags"
   else
     if test "$libssh2" = "yes" ; then
       error_exit "libssh2 >= $min_libssh2_version required for --enable-libssh2"
@@ -2651,9 +2644,6 @@ if test "$glusterfs" != "no" ; then
     glusterfs="yes"
     glusterfs_cflags=`$pkg_config --cflags glusterfs-api`
     glusterfs_libs=`$pkg_config --libs glusterfs-api`
-    CFLAGS="$CFLAGS $glusterfs_cflags"
-    libs_tools="$glusterfs_libs $libs_tools"
-    libs_softmmu="$glusterfs_libs $libs_softmmu"
     if $pkg_config --atleast-version=5 glusterfs-api; then
       glusterfs_discard="yes"
     fi
@@ -3021,11 +3011,9 @@ EOF
     libiscsi="yes"
     libiscsi_cflags=$($pkg_config --cflags libiscsi)
     libiscsi_libs=$($pkg_config --libs libiscsi)
-    CFLAGS="$CFLAGS $libiscsi_cflags"
-    LIBS="$LIBS $libiscsi_libs"
   elif compile_prog "" "-liscsi" ; then
     libiscsi="yes"
-    LIBS="$LIBS -liscsi"
+    libiscsi_libs="-liscsi"
   else
     if test "$libiscsi" = "yes" ; then
       feature_not_found "libiscsi"
@@ -4012,8 +4000,9 @@ if test "$bswap_h" = "yes" ; then
   echo "CONFIG_MACHINE_BSWAP_H=y" >> $config_host_mak
 fi
 if test "$curl" = "yes" ; then
-  echo "CONFIG_CURL=y" >> $config_host_mak
+  echo "CONFIG_CURL=m" >> $config_host_mak
   echo "CURL_CFLAGS=$curl_cflags" >> $config_host_mak
+  echo "CURL_LIBS=$curl_libs" >> $config_host_mak
 fi
 if test "$brlapi" = "yes" ; then
   echo "CONFIG_BRLAPI=y" >> $config_host_mak
@@ -4102,7 +4091,9 @@ if test "$glx" = "yes" ; then
 fi
 
 if test "$libiscsi" = "yes" ; then
-  echo "CONFIG_LIBISCSI=y" >> $config_host_mak
+  echo "CONFIG_LIBISCSI=m" >> $config_host_mak
+  echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
+  echo "LIBISCSI_LIBS=$libiscsi_libs" >> $config_host_mak
 fi
 
 if test "$seccomp" = "yes"; then
@@ -4123,7 +4114,9 @@ if test "$qom_cast_debug" = "yes" ; then
   echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
 fi
 if test "$rbd" = "yes" ; then
-  echo "CONFIG_RBD=y" >> $config_host_mak
+  echo "CONFIG_RBD=m" >> $config_host_mak
+  echo "RBD_CFLAGS=$rbd_cflags" >> $config_host_mak
+  echo "RBD_LIBS=$rbd_libs" >> $config_host_mak
 fi
 
 echo "CONFIG_COROUTINE_BACKEND=$coroutine" >> $config_host_mak
@@ -4161,7 +4154,9 @@ if test "$getauxval" = "yes" ; then
 fi
 
 if test "$glusterfs" = "yes" ; then
-  echo "CONFIG_GLUSTERFS=y" >> $config_host_mak
+  echo "CONFIG_GLUSTERFS=m" >> $config_host_mak
+  echo "GLUSTERFS_CFLAGS=$glusterfs_cflags" >> $config_host_mak
+  echo "GLUSTERFS_LIBS=$glusterfs_libs" >> $config_host_mak
 fi
 
 if test "$glusterfs_discard" = "yes" ; then
@@ -4169,7 +4164,9 @@ if test "$glusterfs_discard" = "yes" ; then
 fi
 
 if test "$libssh2" = "yes" ; then
-  echo "CONFIG_LIBSSH2=y" >> $config_host_mak
+  echo "CONFIG_LIBSSH2=m" >> $config_host_mak
+  echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
+  echo "LIBSSH2_LIBS=$libssh2_libs" >> $config_host_mak
 fi
 
 if test "$virtio_blk_data_plane" = "yes" ; then
-- 
1.8.3.1

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

[Daniel P. Berrange]

On Mon, Sep 16, 2013 at 02:50:24PM +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 whitelisted ".so" files of the given type under ${MODDIR}.
> 
> Modules of each type should be loaded in respective subsystem
> initialization code.
> 
> The init function of dynamic module is no longer with
> __attribute__((constructor)) as static linked version, and need to be
> explicitly called once loaded. The function name is mangled with per
> configure fingerprint as:
> 
>     init_$(date +%s$$$RANDOM)
> 
> Which is known to module_load function, and the loading fails if this
> symbol is not there. With this, modules built from a different
> tree/version/configure will not be loaded.
> 
> The module loading code requires gmodule-2.0.
> 
> Configure option "--enable-modules=L" can be used to restrict qemu to
> only build/load some whitelisted modules.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  Makefile              |  3 ++
>  block.c               |  1 +
>  configure             | 44 +++++++++++++++++++++-------
>  include/qemu/module.h | 23 +++++++++++++++
>  rules.mak             |  9 ++++--
>  scripts/create_config | 22 ++++++++++++++
>  util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                  |  2 ++
>  8 files changed, 172 insertions(+), 13 deletions(-)
> 
> +void module_load(module_load_type type)
> +{
> +#ifdef CONFIG_MODULES
> +    const char *prefix;
> +    char *fname = NULL;
> +    const char **mp;
> +    static const char *module_whitelist[] = {
> +        CONFIG_MODULE_WHITELIST
> +    };
> +
> +    if (!g_module_supported()) {
> +        return;
> +    }
> +
> +    switch (type) {
> +    case MODULE_LOAD_BLOCK:
> +        prefix = "block-";
> +        break;
> +    case MODULE_LOAD_UI:
> +        prefix = "ui-";
> +        break;
> +    case MODULE_LOAD_NET:
> +        prefix = "ui-";

Wrong prefix.

> +        break;
> +    default:
> +        return;
> +    }
> +
> +    for (mp = &module_whitelist[0]; *mp; mp++) {
> +        if (strncmp(prefix, *mp, strlen(prefix))) {
> +            continue;
> +        }
> +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
> +        module_load_file(fname);
> +        g_free(fname);
> +    }
> +
> +#endif
> +}

IMHO this method design is really crazy. If you want to have a
situation where you call module_load() multiple times, then
have separate whitelists for block/net/ui, instead of having
one global whitelist which you then have to load a subset of
each time.  Alternatively just call module_load() once and
give rid of these enums for loading block/net/ui separately.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Fam Zheng]

On Mon, 09/16 09:59, Daniel P. Berrange wrote:
> On Mon, Sep 16, 2013 at 02:50:24PM +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 whitelisted ".so" files of the given type under ${MODDIR}.
> > 
> > Modules of each type should be loaded in respective subsystem
> > initialization code.
> > 
> > The init function of dynamic module is no longer with
> > __attribute__((constructor)) as static linked version, and need to be
> > explicitly called once loaded. The function name is mangled with per
> > configure fingerprint as:
> > 
> >     init_$(date +%s$$$RANDOM)
> > 
> > Which is known to module_load function, and the loading fails if this
> > symbol is not there. With this, modules built from a different
> > tree/version/configure will not be loaded.
> > 
> > The module loading code requires gmodule-2.0.
> > 
> > Configure option "--enable-modules=L" can be used to restrict qemu to
> > only build/load some whitelisted modules.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  Makefile              |  3 ++
> >  block.c               |  1 +
> >  configure             | 44 +++++++++++++++++++++-------
> >  include/qemu/module.h | 23 +++++++++++++++
> >  rules.mak             |  9 ++++--
> >  scripts/create_config | 22 ++++++++++++++
> >  util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                  |  2 ++
> >  8 files changed, 172 insertions(+), 13 deletions(-)
> > 
> > +void module_load(module_load_type type)
> > +{
> > +#ifdef CONFIG_MODULES
> > +    const char *prefix;
> > +    char *fname = NULL;
> > +    const char **mp;
> > +    static const char *module_whitelist[] = {
> > +        CONFIG_MODULE_WHITELIST
> > +    };
> > +
> > +    if (!g_module_supported()) {
> > +        return;
> > +    }
> > +
> > +    switch (type) {
> > +    case MODULE_LOAD_BLOCK:
> > +        prefix = "block-";
> > +        break;
> > +    case MODULE_LOAD_UI:
> > +        prefix = "ui-";
> > +        break;
> > +    case MODULE_LOAD_NET:
> > +        prefix = "ui-";
> 
> Wrong prefix.
> 
> > +        break;
> > +    default:
> > +        return;
> > +    }
> > +
> > +    for (mp = &module_whitelist[0]; *mp; mp++) {
> > +        if (strncmp(prefix, *mp, strlen(prefix))) {
> > +            continue;
> > +        }
> > +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
> > +        module_load_file(fname);
> > +        g_free(fname);
> > +    }
> > +
> > +#endif
> > +}
> 
> IMHO this method design is really crazy. If you want to have a
> situation where you call module_load() multiple times, then
> have separate whitelists for block/net/ui, instead of having
> one global whitelist which you then have to load a subset of
> each time.  Alternatively just call module_load() once and
> give rid of these enums for loading block/net/ui separately.
> 

It's pretty clear that we have different subsets of modules to load for target
emulator and qemu-img, so not having enums is not working.

What's the disadvantage of this, and why are separate lists better?

Thanks,
Fam

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

[Paolo Bonzini]

Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>> The init function of dynamic module is no longer with
>> __attribute__((constructor)) as static linked version, and need to be
>> explicitly called once loaded. The function name is mangled with per
>> configure fingerprint as:
>> 
>>     init_$(date +%s$$$RANDOM)

Does this work for a module that calls module_init multiple times?

Paolo

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

[Fam Zheng]

On Mon, 09/16 11:44, Paolo Bonzini wrote:
> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >> The init function of dynamic module is no longer with
> >> __attribute__((constructor)) as static linked version, and need to be
> >> explicitly called once loaded. The function name is mangled with per
> >> configure fingerprint as:
> >> 
> >>     init_$(date +%s$$$RANDOM)
> 
> Does this work for a module that calls module_init multiple times?
> 
Why should a module calls module_init, instead of the main function?

This name is generated per "./configure", not per object or per make, so it's
essentially the same with any fixed function name, except for two objects built
from two different "./configure" (which is the purpose for the mangling here).

Does this answer your question?

Fam

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

[Paolo Bonzini]

Il 16/09/2013 11:51, Fam Zheng ha scritto:
> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>> The init function of dynamic module is no longer with
>>>> __attribute__((constructor)) as static linked version, and need to be
>>>> explicitly called once loaded. The function name is mangled with per
>>>> configure fingerprint as:
>>>>
>>>>     init_$(date +%s$$$RANDOM)
>>
>> Does this work for a module that calls module_init multiple times?
>
> Why should a module calls module_init, instead of the main function?

I think you mean "why should a module calls register_module_init", and I
agree that with this patch a module will not call register_module_init.

But a module is still using the module_init macro.

With this patch, a module will not be able to use the module_init macro
twice.  I am not sure this is an acceptable limitation, especially if we
do not have a dependency system within modules and/or load them with
G_MODULE_LOCAL/RTLD_LOCAL.

Paolo

> This name is generated per "./configure", not per object or per make, so it's
> essentially the same with any fixed function name, except for two objects built
> from two different "./configure" (which is the purpose for the mangling here).
> 
> Does this answer your question?
> 
> Fam
> 

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

[Daniel P. Berrange]

On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> > On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>> The init function of dynamic module is no longer with
> >>>> __attribute__((constructor)) as static linked version, and need to be
> >>>> explicitly called once loaded. The function name is mangled with per
> >>>> configure fingerprint as:
> >>>>
> >>>>     init_$(date +%s$$$RANDOM)
> >>
> >> Does this work for a module that calls module_init multiple times?
> >
> > Why should a module calls module_init, instead of the main function?
> 
> I think you mean "why should a module calls register_module_init", and I
> agree that with this patch a module will not call register_module_init.
> 
> But a module is still using the module_init macro.
> 
> With this patch, a module will not be able to use the module_init macro
> twice.  I am not sure this is an acceptable limitation, especially if we
> do not have a dependency system within modules and/or load them with
> G_MODULE_LOCAL/RTLD_LOCAL.

Why would a module ever want to use the module_init macro twice ? IIUC
this function is supposed todo one-time initialization work for the .so
module. Surely any place where a module wanted to use module_init twice
could be solved by having that module put all its init logic into just
one function. So I'm not sure I see where the problem is.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Paolo Bonzini]

Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>> The init function of dynamic module is no longer with
>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>> configure fingerprint as:
>>>>>>
>>>>>>     init_$(date +%s$$$RANDOM)
>>>>
>>>> Does this work for a module that calls module_init multiple times?
>>>
>>> Why should a module calls module_init, instead of the main function?
>>
>> I think you mean "why should a module calls register_module_init", and I
>> agree that with this patch a module will not call register_module_init.
>>
>> But a module is still using the module_init macro.
>>
>> With this patch, a module will not be able to use the module_init macro
>> twice.  I am not sure this is an acceptable limitation, especially if we
>> do not have a dependency system within modules and/or load them with
>> G_MODULE_LOCAL/RTLD_LOCAL.
> 
> Why would a module ever want to use the module_init macro twice ?

Because our coding standard is to have each source file do its own
one-time initialization, using static functions and an invocation of
module_init per source file.

The reason is that otherwise you risk having function name conflicts in
the static-link case.

Paolo

> IIUC
> this function is supposed todo one-time initialization work for the .so
> module. Surely any place where a module wanted to use module_init twice
> could be solved by having that module put all its init logic into just
> one function. So I'm not sure I see where the problem is.

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

[Daniel P. Berrange]

On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> > On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> >>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>>>> The init function of dynamic module is no longer with
> >>>>>> __attribute__((constructor)) as static linked version, and need to be
> >>>>>> explicitly called once loaded. The function name is mangled with per
> >>>>>> configure fingerprint as:
> >>>>>>
> >>>>>>     init_$(date +%s$$$RANDOM)
> >>>>
> >>>> Does this work for a module that calls module_init multiple times?
> >>>
> >>> Why should a module calls module_init, instead of the main function?
> >>
> >> I think you mean "why should a module calls register_module_init", and I
> >> agree that with this patch a module will not call register_module_init.
> >>
> >> But a module is still using the module_init macro.
> >>
> >> With this patch, a module will not be able to use the module_init macro
> >> twice.  I am not sure this is an acceptable limitation, especially if we
> >> do not have a dependency system within modules and/or load them with
> >> G_MODULE_LOCAL/RTLD_LOCAL.
> > 
> > Why would a module ever want to use the module_init macro twice ?
> 
> Because our coding standard is to have each source file do its own
> one-time initialization, using static functions and an invocation of
> module_init per source file.

Is there ever a case where two source files, each using module_init
will be compiled into the same .so loadable module. Looking at the
uses of block_init(), I don't see any obvious candidates for trouble,
all uses look like they'd be going into separate .so files.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Alex Bligh]

On 16 Sep 2013, at 10:51, Fam Zheng wrote:

> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>> The init function of dynamic module is no longer with
>>>> __attribute__((constructor)) as static linked version, and need to be
>>>> explicitly called once loaded. The function name is mangled with per
>>>> configure fingerprint as:
>>>> 
>>>>    init_$(date +%s$$$RANDOM)
>> 
>> Does this work for a module that calls module_init multiple times?
>> 
> Why should a module calls module_init, instead of the main function?
> 
> This name is generated per "./configure", not per object or per make, so it's
> essentially the same with any fixed function name, except for two objects built
> from two different "./configure" (which is the purpose for the mangling here).

I think I must be missing something here.

We do not have a stable API/ABI and it seems generally acknowledged at this
stage that we don't need one. Therefore, to avoid API/ABI mismatch between
the executables and the modules, in ./configure you are generating a random
cookie (effectively) that you are calling the fingerprint.

The executable will then not load the module unless the module has the
right cookie. As far as I can tell, that means the module needs to be
built within the same build harness as the executables, or it won't
know what to call its init function.

And that's perfectly compatible with the stated objective:
> The main idea behind modules is to isolate dependencies on third party
> libraries from qemu executables, such as libglusterfs or librbd, so that the
> end users can install core qemu package with fewer dependencies.  And only for
> those who want to use particular modules, need they install qemu-foo
> sub-package, which in turn requires libbar and libbiz packages.

... this being to isolate dependencies, and not to enable third party
modules built outside the tree.

That's all well and good, but if the modules are all built within the
same build harness, why do we need a whitelist or a readdir() at all? We
know what the modules are, because they were the ones that were built
at the same time. Why not just process the list of modules it was
built with, and if you get EEXIST, move on?

The other issue I have with this approach is that it doesn't address
the situation where the distro builds the qemu version against its
distributed version of the development library for the plug in, but
on deployment, everyone wants to use a new development library. To
give a concrete example, qemu in Ubuntu Precise is build against
librbd 0.42 (from memory - don't shoot me if wrong), and that supports
fewer calls than librbd 0.62 which is what anyone sane will want to
run.

At risk of heresy, can I suggest a rather simpler scheme that requires
a total of zero infrastructure changes?

Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
uses weak binding to load and compile against any version of
librbd:
  https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960

This requires librbd-dev headers of some sort in order to build. But
librbd does not need to be there on a deployment, and indeed multiple
versions work (and the patch does different things depending on
librbd version).

This would seem to achieve the stated objective (qemu package can
be installed without librbd dependencies) without any reliance
on modules at all. So we could scrap the whole modules infrastructure
and just tell people developing third party libraries to use weak
binding. As you can see, for librbd anyway, the changes required
to support weak binding are pretty minimal.

-- 
Alex Bligh

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

[Paolo Bonzini]

Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
> On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
>>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>>>> The init function of dynamic module is no longer with
>>>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>>>> configure fingerprint as:
>>>>>>>>
>>>>>>>>     init_$(date +%s$$$RANDOM)
>>>>>>
>>>>>> Does this work for a module that calls module_init multiple times?
>>>>>
>>>>> Why should a module calls module_init, instead of the main function?
>>>>
>>>> I think you mean "why should a module calls register_module_init", and I
>>>> agree that with this patch a module will not call register_module_init.
>>>>
>>>> But a module is still using the module_init macro.
>>>>
>>>> With this patch, a module will not be able to use the module_init macro
>>>> twice.  I am not sure this is an acceptable limitation, especially if we
>>>> do not have a dependency system within modules and/or load them with
>>>> G_MODULE_LOCAL/RTLD_LOCAL.
>>>
>>> Why would a module ever want to use the module_init macro twice ?
>>
>> Because our coding standard is to have each source file do its own
>> one-time initialization, using static functions and an invocation of
>> module_init per source file.
> 
> Is there ever a case where two source files, each using module_init
> will be compiled into the same .so loadable module. Looking at the
> uses of block_init(), I don't see any obvious candidates for trouble,
> all uses look like they'd be going into separate .so files.

Without inter-module exports, all of SPICE probably would have to be in
a single .so file.  This includes spice-qemu-char.c and
hw/display/qxl.c, both of which use type_init.

If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
then indeed this is a much smaller problem, but then we need a
dependency system.  But I'm almost sure that Windows and maybe Darwin
lack support for the equivalent of G_MODULE_GLOBAL.

BTW, we need a buildbot for the static linking case, otherwise as
modules become more widespread, we'll have hard to detect bugs due to
duplicate symbols.

Paolo

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

[Paolo Bonzini]

Il 16/09/2013 12:24, Alex Bligh ha scritto:
> At risk of heresy, can I suggest a rather simpler scheme that requires
> a total of zero infrastructure changes?
> 
> Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
> uses weak binding to load and compile against any version of
> librbd:
>   https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960
> 
> This requires librbd-dev headers of some sort in order to build. But
> librbd does not need to be there on a deployment, and indeed multiple
> versions work (and the patch does different things depending on
> librbd version).

No, librbd does need to be there for the other symbols that are not weak
(e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
removing the librbd dependency altogether.  For example:

xx.c:
int f(void)
{
	return 42;
}

yy.c:
#pragma weak f
extern int f(void);
int main()
{
	printf("%p %d", f, f ? f(): 67);
}

$ gcc xx.c -shared -o xx.so
$ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
$ ./yy
0x4005b0 42
$ rm xx.so
$ ./yy
./yy: error while loading shared libraries: xx.so: cannot open shared
object file: No such file or directory


Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
becomes unmaintainable.

Paolo

> This would seem to achieve the stated objective (qemu package can
> be installed without librbd dependencies) without any reliance
> on modules at all. So we could scrap the whole modules infrastructure
> and just tell people developing third party libraries to use weak
> binding. As you can see, for librbd anyway, the changes required
> to support weak binding are pretty minimal.

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

[Fam Zheng]

On Mon, 09/16 11:24, Alex Bligh wrote:
> 
> On 16 Sep 2013, at 10:51, Fam Zheng wrote:
> 
> > On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>> The init function of dynamic module is no longer with
> >>>> __attribute__((constructor)) as static linked version, and need to be
> >>>> explicitly called once loaded. The function name is mangled with per
> >>>> configure fingerprint as:
> >>>> 
> >>>>    init_$(date +%s$$$RANDOM)
> >> 
> >> Does this work for a module that calls module_init multiple times?
> >> 
> > Why should a module calls module_init, instead of the main function?
> > 
> > This name is generated per "./configure", not per object or per make, so it's
> > essentially the same with any fixed function name, except for two objects built
> > from two different "./configure" (which is the purpose for the mangling here).
> 
> I think I must be missing something here.
> 
> We do not have a stable API/ABI and it seems generally acknowledged at this
> stage that we don't need one. Therefore, to avoid API/ABI mismatch between
> the executables and the modules, in ./configure you are generating a random
> cookie (effectively) that you are calling the fingerprint.
> 
> The executable will then not load the module unless the module has the
> right cookie. As far as I can tell, that means the module needs to be
> built within the same build harness as the executables, or it won't
> know what to call its init function.
> 
> And that's perfectly compatible with the stated objective:
> > The main idea behind modules is to isolate dependencies on third party
> > libraries from qemu executables, such as libglusterfs or librbd, so that the
> > end users can install core qemu package with fewer dependencies.  And only for
> > those who want to use particular modules, need they install qemu-foo
> > sub-package, which in turn requires libbar and libbiz packages.
> 
> ... this being to isolate dependencies, and not to enable third party
> modules built outside the tree.
> 
> That's all well and good, but if the modules are all built within the
> same build harness, why do we need a whitelist or a readdir() at all? We
> know what the modules are, because they were the ones that were built
> at the same time. Why not just process the list of modules it was
> built with, and if you get EEXIST, move on?
> 

Sounds good to me, I agree that "whitelist" is not necessary for user. We still
need a known_modules in the new module_load() code and it'll look very similar
to current whitelist. But I'll change the name.

Fam

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

[Gerd Hoffmann]

  Hi,

> With this patch, a module will not be able to use the module_init macro
> twice.  I am not sure this is an acceptable limitation, especially if we
> do not have a dependency system within modules and/or load them with
> G_MODULE_LOCAL/RTLD_LOCAL.

Exactly.  To modularize spice we need either inter-module dependencies,
so spice-audio.mo can depend on spice-core.mo etc, or allow multiple
module_init calls so we can link all spice components into one big
spice.mo module and each component can use module_init.

It's not mandatory for the initial revision, we'll need a bit more (like
registering monitor commands for 'info spice') so we can actually
modularize spice.  But it should definitively on the radar for the
planning ...

cheers,
  Gerd

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

[Paolo Bonzini]

Il 16/09/2013 12:57, Gerd Hoffmann ha scritto:
>   Hi,
> 
>> With this patch, a module will not be able to use the module_init macro
>> twice.  I am not sure this is an acceptable limitation, especially if we
>> do not have a dependency system within modules and/or load them with
>> G_MODULE_LOCAL/RTLD_LOCAL.
> 
> Exactly.  To modularize spice we need either inter-module dependencies,
> so spice-audio.mo can depend on spice-core.mo etc, or allow multiple
> module_init calls so we can link all spice components into one big
> spice.mo module and each component can use module_init.

We could also have a huge web of shared objects like LibreOffice has
(spice-core.so depending on qemu-system.so, and spice.mo depending on
spice-core.so), but I'm not really suggesting that...

Paolo

> It's not mandatory for the initial revision, we'll need a bit more (like
> registering monitor commands for 'info spice') so we can actually
> modularize spice.  But it should definitively on the radar for the
> planning ...
> 
> cheers,
>   Gerd
> 
> 
> 

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

[Alex Bligh]

On 16 Sep 2013, at 11:38, Paolo Bonzini wrote:

> No, librbd does need to be there for the other symbols that are not weak
> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
> removing the librbd dependency altogether.  For example:
> 
> xx.c:
> int f(void)
> {
> 	return 42;
> }
> 
> yy.c:
> #pragma weak f
> extern int f(void);
> int main()
> {
> 	printf("%p %d", f, f ? f(): 67);
> }
> 
> $ gcc xx.c -shared -o xx.so
> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
> $ ./yy
> 0x4005b0 42
> $ rm xx.so
> $ ./yy
> ./yy: error while loading shared libraries: xx.so: cannot open shared
> object file: No such file or directory

I think you need to wrap f, i.e. take g as a pointer to f(), and
call g().

> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
> becomes unmaintainable.

I agree it's ugly. However, it's pretty much the only way to cope
with different versions of libraries.

However, even if you don't use weak symbols, we could simply dlopen()
a fixed list of modules known at compile time from a single directory
(because we also know at compile which executable needs what, e.g.
that qemu-img doesn't need spice or whatever).

-- 
Alex Bligh

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

[Daniel P. Berrange]

On Mon, Sep 16, 2013 at 12:00:47PM +0100, Alex Bligh wrote:
> 
> However, even if you don't use weak symbols, we could simply dlopen()
> a fixed list of modules known at compile time from a single directory
> (because we also know at compile which executable needs what, e.g.
> that qemu-img doesn't need spice or whatever).

That's what this latest patch series already does, albeit with some
redundant complexity of trying to split the modules into UI/Net/Block
lists.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Daniel P. Berrange]

On Mon, Sep 16, 2013 at 02:50:24PM +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 whitelisted ".so" files of the given type under ${MODDIR}.
> 
> Modules of each type should be loaded in respective subsystem
> initialization code.

Based on Paolo's note that the SPICE .so module could likely
end up containing functionality that is spread across several
different sub-systems, this approach of loading per-type
seems even more flawed. I think I'd just have one flat list
of modules to load and ditch these  MODULE_LOAD_XXXX enums.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Paolo Bonzini]

Il 16/09/2013 13:00, Alex Bligh ha scritto:
> 
> On 16 Sep 2013, at 11:38, Paolo Bonzini wrote:
> 
>> No, librbd does need to be there for the other symbols that are not weak
>> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
>> removing the librbd dependency altogether.  For example:
>>
>> xx.c:
>> int f(void)
>> {
>> 	return 42;
>> }
>>
>> yy.c:
>> #pragma weak f
>> extern int f(void);
>> int main()
>> {
>> 	printf("%p %d", f, f ? f(): 67);
>> }
>>
>> $ gcc xx.c -shared -o xx.so
>> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
>> $ ./yy
>> 0x4005b0 42
>> $ rm xx.so
>> $ ./yy
>> ./yy: error while loading shared libraries: xx.so: cannot open shared
>> object file: No such file or directory
> 
> I think you need to wrap f, i.e. take g as a pointer to f(), and
> call g().

No, you need to do dlopen("librbd.so"), which is bad because then
distros that can track .so dependencies will not do it anymore.
qemu-devel had a patch to do exactly that.

>> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
>> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
>> becomes unmaintainable.
> 
> I agree it's ugly. However, it's pretty much the only way to cope
> with different versions of libraries.

But the reason to do modularization is not to "cope with different
versions of libraries".  In fact that's a problem that Fam's patches do
not solve at all.  The reason to do modularization is to make libraries
optional, i.e. let them be completely absent.

Paolo

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

[Alex Bligh]

On 16 Sep 2013, at 12:04, Daniel P. Berrange wrote:

> On Mon, Sep 16, 2013 at 12:00:47PM +0100, Alex Bligh wrote:
>> 
>> However, even if you don't use weak symbols, we could simply dlopen()
>> a fixed list of modules known at compile time from a single directory
>> (because we also know at compile which executable needs what, e.g.
>> that qemu-img doesn't need spice or whatever).
> 
> That's what this latest patch series already does, albeit with some
> redundant complexity of trying to split the modules into UI/Net/Block
> lists.

Last time I looked (sorry if I'm out of date), it had whitelists
(which implies you can load things other than on the whitelists),
nested directories (as above), and readdir(). None of these are
necessary if the complete list of modules is known at build time.
And using the mangling method we're using at the moment, no
modules can be built other than at build time.

-- 
Alex Bligh

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

[Fam Zheng]

On Mon, 09/16 12:30, Paolo Bonzini wrote:
> Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
> > On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
> >>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
> >>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
> >>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
> >>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
> >>>>>>>> The init function of dynamic module is no longer with
> >>>>>>>> __attribute__((constructor)) as static linked version, and need to be
> >>>>>>>> explicitly called once loaded. The function name is mangled with per
> >>>>>>>> configure fingerprint as:
> >>>>>>>>
> >>>>>>>>     init_$(date +%s$$$RANDOM)
> >>>>>>
> >>>>>> Does this work for a module that calls module_init multiple times?
> >>>>>
> >>>>> Why should a module calls module_init, instead of the main function?
> >>>>
> >>>> I think you mean "why should a module calls register_module_init", and I
> >>>> agree that with this patch a module will not call register_module_init.
> >>>>
> >>>> But a module is still using the module_init macro.
> >>>>
> >>>> With this patch, a module will not be able to use the module_init macro
> >>>> twice.  I am not sure this is an acceptable limitation, especially if we
> >>>> do not have a dependency system within modules and/or load them with
> >>>> G_MODULE_LOCAL/RTLD_LOCAL.
> >>>
> >>> Why would a module ever want to use the module_init macro twice ?
> >>
> >> Because our coding standard is to have each source file do its own
> >> one-time initialization, using static functions and an invocation of
> >> module_init per source file.
> > 
> > Is there ever a case where two source files, each using module_init
> > will be compiled into the same .so loadable module. Looking at the
> > uses of block_init(), I don't see any obvious candidates for trouble,
> > all uses look like they'd be going into separate .so files.
> 
> Without inter-module exports, all of SPICE probably would have to be in
> a single .so file.  This includes spice-qemu-char.c and
> hw/display/qxl.c, both of which use type_init.
> 
> If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
> then indeed this is a much smaller problem, but then we need a
> dependency system.  But I'm almost sure that Windows and maybe Darwin
> lack support for the equivalent of G_MODULE_GLOBAL.
> 

An idea for single .so file:
    - before loads a .so, an empty initializer list is created.
    - module_init adds a __attribute__((constructor)) function, which appends
      its real initializer to the initializer list. So this function is
      automatically called after dlopen().
    - make init_$(date +%s$$$RANDOM) a dummy symbol.
    - module_load first checks the presense of the symbol, if yes, call the
      functions in the initializer list. Else clean up and unload .so.

Does this enable multiple calls of module_init()?

OTOH. As for multiple spice modules, is it possible to solve it by having a
spice-common.o and link all spice modules to it, to share code?

Fam

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

[Alex Bligh]

On 16 Sep 2013, at 12:08, Paolo Bonzini wrote:

> But the reason to do modularization is not to "cope with different
> versions of libraries".  In fact that's a problem that Fam's patches do
> not solve at all.  The reason to do modularization is to make libraries
> optional, i.e. let them be completely absent.

You are right. In fact the 'different versions of libraries' thing is
largely an orthogonal issue as a modular or non-modualar rbd driver
would have much the same set of issues here.

Moreover, one Fam's patches (hopefully simplified) could still help
here by permitting (e.g.) a blkrbd-new.so & blkrbd-old.so (hopefully
with more useful names), for those with an allergy to weak binding.

-- 
Alex Bligh

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

[Paolo Bonzini]

Il 16/09/2013 13:29, Fam Zheng ha scritto:
> An idea for single .so file:
>     - before loads a .so, an empty initializer list is created.
>     - module_init adds a __attribute__((constructor)) function, which appends
>       its real initializer to the initializer list. So this function is
>       automatically called after dlopen().
>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
>     - module_load first checks the presense of the symbol, if yes, call the
>       functions in the initializer list. Else clean up and unload .so.
> 
> Does this enable multiple calls of module_init()?

Yes.  Basically you are delaying the constructors; that would work.

> OTOH. As for multiple spice modules, is it possible to solve it by having a
> spice-common.o and link all spice modules to it, to share code?

Looks like there is global state in ui/spice-core.c, so likely no.

Paolo

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

[Fam Zheng]

On Mon, 09/16 13:33, Paolo Bonzini wrote:
> Il 16/09/2013 13:29, Fam Zheng ha scritto:
> > An idea for single .so file:
> >     - before loads a .so, an empty initializer list is created.
> >     - module_init adds a __attribute__((constructor)) function, which appends
> >       its real initializer to the initializer list. So this function is
> >       automatically called after dlopen().
> >     - make init_$(date +%s$$$RANDOM) a dummy symbol.
> >     - module_load first checks the presense of the symbol, if yes, call the
> >       functions in the initializer list. Else clean up and unload .so.
> > 
> > Does this enable multiple calls of module_init()?
> 
> Yes.  Basically you are delaying the constructors; that would work.
> 
> > OTOH. As for multiple spice modules, is it possible to solve it by having a
> > spice-common.o and link all spice modules to it, to share code?
> 
> Looks like there is global state in ui/spice-core.c, so likely no.
> 

What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
call require_module("spice-core") before initializing itself, which will load
this dependency if not yet.  This may be the simplest implementation of
dependency resolving.

Fam

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

[Gerd Hoffmann]

On Mo, 2013-09-16 at 12:05 +0100, Daniel P. Berrange wrote:
> On Mon, Sep 16, 2013 at 02:50:24PM +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 whitelisted ".so" files of the given type under ${MODDIR}.
> > 
> > Modules of each type should be loaded in respective subsystem
> > initialization code.
> 
> Based on Paolo's note that the SPICE .so module could likely
> end up containing functionality that is spread across several
> different sub-systems, this approach of loading per-type
> seems even more flawed.

spice would need different types indeed (and the list above looks
incomplete).

> I think I'd just have one flat list
> of modules to load and ditch these  MODULE_LOAD_XXXX enums.

Question is how to deal with qemu vs. qemu-img then.  qemu needs
everything and qemu-img needs the block drivers only (and loading
something else probably doesn't work due to unresolved symbols).

cheers,
  Gerd

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

[Richard Henderson]

On 09/16/2013 02:28 AM, Fam Zheng wrote:
> What's the disadvantage of this, and why are separate lists better?

You're performing useless work, making the code more confusing in the process.
 How can it not be better to have separate lists?


r~

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

[Richard Henderson]

On 09/16/2013 04:46 AM, Fam Zheng wrote:
> On Mon, 09/16 13:33, Paolo Bonzini wrote:
>> Il 16/09/2013 13:29, Fam Zheng ha scritto:
>>> An idea for single .so file:
>>>     - before loads a .so, an empty initializer list is created.
>>>     - module_init adds a __attribute__((constructor)) function, which appends
>>>       its real initializer to the initializer list. So this function is
>>>       automatically called after dlopen().
>>>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
>>>     - module_load first checks the presense of the symbol, if yes, call the
>>>       functions in the initializer list. Else clean up and unload .so.
>>>
>>> Does this enable multiple calls of module_init()?
>>
>> Yes.  Basically you are delaying the constructors; that would work.
>>
>>> OTOH. As for multiple spice modules, is it possible to solve it by having a
>>> spice-common.o and link all spice modules to it, to share code?
>>
>> Looks like there is global state in ui/spice-core.c, so likely no.
>>
> 
> What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
> call require_module("spice-core") before initializing itself, which will load
> this dependency if not yet.  This may be the simplest implementation of
> dependency resolving.

Why oh why would you want to re-invent the dependencies that ld.so already
provides?

Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
recorded, and ld.so will do the right thing.

Anything else sounds way too much like Not Invented Here.


r~

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

[Richard Henderson]

On 09/16/2013 04:00 AM, Paolo Bonzini wrote:
> We could also have a huge web of shared objects like LibreOffice has
> (spice-core.so depending on qemu-system.so, and spice.mo depending on
> spice-core.so), but I'm not really suggesting that...

I am.  Although in our case I wouldn't expect the web to be huge.

To me it seems like working with the system dynamic linker is easier than
re-inventing our own.


r~

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

[Fam Zheng]

On Mon, 09/16 15:16, Richard Henderson wrote:
> On 09/16/2013 02:28 AM, Fam Zheng wrote:
> > What's the disadvantage of this, and why are separate lists better?
> 
> You're performing useless work, making the code more confusing in the process.
>  How can it not be better to have separate lists?
> 
> 
Because it's about adding more code in Makefile to separate them and adding
more code here to define and use multiple lists. I am not sure it's much more
readable than this, but it's really non critical question, let me first try to
do it.

Thanks,

Fam

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

[Fam Zheng]

On Mon, 09/16 15:31, Richard Henderson wrote:
> On 09/16/2013 04:46 AM, Fam Zheng wrote:
> > On Mon, 09/16 13:33, Paolo Bonzini wrote:
> >> Il 16/09/2013 13:29, Fam Zheng ha scritto:
> >>> An idea for single .so file:
> >>>     - before loads a .so, an empty initializer list is created.
> >>>     - module_init adds a __attribute__((constructor)) function, which appends
> >>>       its real initializer to the initializer list. So this function is
> >>>       automatically called after dlopen().
> >>>     - make init_$(date +%s$$$RANDOM) a dummy symbol.
> >>>     - module_load first checks the presense of the symbol, if yes, call the
> >>>       functions in the initializer list. Else clean up and unload .so.
> >>>
> >>> Does this enable multiple calls of module_init()?
> >>
> >> Yes.  Basically you are delaying the constructors; that would work.
> >>
> >>> OTOH. As for multiple spice modules, is it possible to solve it by having a
> >>> spice-common.o and link all spice modules to it, to share code?
> >>
> >> Looks like there is global state in ui/spice-core.c, so likely no.
> >>
> > 
> > What if the modules can be loaded by name? Then in spice-qemu-char.so, it can
> > call require_module("spice-core") before initializing itself, which will load
> > this dependency if not yet.  This may be the simplest implementation of
> > dependency resolving.
> 
> Why oh why would you want to re-invent the dependencies that ld.so already
> provides?
> 
> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
> recorded, and ld.so will do the right thing.
> 
> Anything else sounds way too much like Not Invented Here.
> 
How to do the symbol checking as above if spice-core.so is automatically loaded
by ld.so?

And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
modules directory too.

Fam

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

[Richard Henderson]

On 09/16/2013 06:29 PM, Fam Zheng wrote:
>> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
>> recorded, and ld.so will do the right thing.
>>
>> Anything else sounds way too much like Not Invented Here.
>>
> How to do the symbol checking as above if spice-core.so is automatically loaded
> by ld.so?

You're checking the version stamp in spice-char.so.  I'd think that's good
enough.  No need to transitively check.

> And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
> modules directory too.

Or add DT_RUN_PATH to the main executable, or even add DT_RUN_PATH to the
module itself.  In particular, link the module with

  -Wl,--enable-new-dtags -Wl,-rpath,'$ORIGIN'

and dependencies for the module will automatically be looked for in the
directory in which the module is found.  Which is almost certiainly the only
thing that we want -- all modules in the same directory.


r~

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

[Fam Zheng]

On Mon, 09/16 14:36, Gerd Hoffmann wrote:
> On Mo, 2013-09-16 at 12:05 +0100, Daniel P. Berrange wrote:
> > On Mon, Sep 16, 2013 at 02:50:24PM +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 whitelisted ".so" files of the given type under ${MODDIR}.
> > > 
> > > Modules of each type should be loaded in respective subsystem
> > > initialization code.
> > 
> > Based on Paolo's note that the SPICE .so module could likely
> > end up containing functionality that is spread across several
> > different sub-systems, this approach of loading per-type
> > seems even more flawed.
> 
> spice would need different types indeed (and the list above looks
> incomplete).
> 
> > I think I'd just have one flat list
> > of modules to load and ditch these  MODULE_LOAD_XXXX enums.
> 
> Question is how to deal with qemu vs. qemu-img then.  qemu needs
> everything and qemu-img needs the block drivers only (and loading
> something else probably doesn't work due to unresolved symbols).
> 

With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
problem to load them ahead.

Fam

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

[Alex Bligh]

On 17 Sep 2013, at 06:55, Fam Zheng wrote:

>>> 
>>> I think I'd just have one flat list
>>> of modules to load and ditch these  MODULE_LOAD_XXXX enums.
>> 
>> Question is how to deal with qemu vs. qemu-img then.  qemu needs
>> everything and qemu-img needs the block drivers only (and loading
>> something else probably doesn't work due to unresolved symbols).
>> 
> 
> With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
> modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
> the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
> problem to load them ahead.

Why not have one list per executable you build? You know at compile
time what modules each executable could load. This seems better
than putting the logic in and run time and having different types
of modules etc.

One reason to avoid qemu-img (for instance) loading everything (if
it's present) is init time. I agree dlopen()'ing something that
never gets called should not eat too much RAM but it seems pointless.

-- 
Alex Bligh

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

[Fam Zheng]

On Tue, 09/17 07:33, Alex Bligh wrote:
> 
> On 17 Sep 2013, at 06:55, Fam Zheng wrote:
> 
> >>> 
> >>> I think I'd just have one flat list
> >>> of modules to load and ditch these  MODULE_LOAD_XXXX enums.
> >> 
> >> Question is how to deal with qemu vs. qemu-img then.  qemu needs
> >> everything and qemu-img needs the block drivers only (and loading
> >> something else probably doesn't work due to unresolved symbols).
> >> 
> > 
> > With lazy symbol binding (G_MODULE_BIND_LAZY), we can just load all the
> > modules, and wait for subsystem to call module_call_init(MODULE_INIT_*), where
> > the symbols are resolved. As qemu-img.c doesn't init ui, net, it's not a
> > problem to load them ahead.
> 
> Why not have one list per executable you build? You know at compile
> time what modules each executable could load. This seems better
> than putting the logic in and run time and having different types
> of modules etc.
> 
> One reason to avoid qemu-img (for instance) loading everything (if
> it's present) is init time. I agree dlopen()'ing something that
> never gets called should not eat too much RAM but it seems pointless.
> 

OK, good point.

Fam

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

[Wenchao Xia]

于 2013/9/16 18:38, Paolo Bonzini 写道:
> Il 16/09/2013 12:24, Alex Bligh ha scritto:
>> At risk of heresy, can I suggest a rather simpler scheme that requires
>> a total of zero infrastructure changes?
>>
>> Here's a patch against qemu 1.0 (sorry) Ubuntu dist (sorry) that
>> uses weak binding to load and compile against any version of
>> librbd:
>>    https://github.com/flexiant/qemu/commit/6fa2e9c95bdaca7c814881e27f04424fb6cc2960
>>
>> This requires librbd-dev headers of some sort in order to build. But
>> librbd does not need to be there on a deployment, and indeed multiple
>> versions work (and the patch does different things depending on
>> librbd version).
> No, librbd does need to be there for the other symbols that are not weak
> (e.g. rbd_aio_read).  This approach cannot be "taken to the limit", i.e.
> removing the librbd dependency altogether.  For example:
>
> xx.c:
> int f(void)
> {
> 	return 42;
> }
>
> yy.c:
> #pragma weak f
> extern int f(void);
> int main()
> {
> 	printf("%p %d", f, f ? f(): 67);
> }
>
> $ gcc xx.c -shared -o xx.so
> $ LD_RUN_PATH=$PWD gcc yy.c xx.so -o yy
> $ ./yy
> 0x4005b0 42
> $ rm xx.so
> $ ./yy
> ./yy: error while loading shared libraries: xx.so: cannot open shared
> object file: No such file or directory
>
Is there a way to tell the program loader: don't resolve the dynamic 
symbol, f,
then code can run into main(), and we add code do 
dlopen("/another_dir/xx.so")? (Assume xx.so
exist in /another_dir/?

> Also, the code _is_ ugly.  Do it once and it's perhaps acceptable.  Do
> it for libiscsi, librbd, libcurl, libssh2, SPICE, GTK+, SDL etc. and it
> becomes unmaintainable.
>
> Paolo
>
>> This would seem to achieve the stated objective (qemu package can
>> be installed without librbd dependencies) without any reliance
>> on modules at all. So we could scrap the whole modules infrastructure
>> and just tell people developing third party libraries to use weak
>> binding. As you can see, for librbd anyway, the changes required
>> to support weak binding are pretty minimal.
>
>

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

[Wenchao Xia]

于 2013/9/16 19:29, Fam Zheng 写道:
> On Mon, 09/16 12:30, Paolo Bonzini wrote:
>> Il 16/09/2013 12:21, Daniel P. Berrange ha scritto:
>>> On Mon, Sep 16, 2013 at 12:18:54PM +0200, Paolo Bonzini wrote:
>>>> Il 16/09/2013 12:14, Daniel P. Berrange ha scritto:
>>>>> On Mon, Sep 16, 2013 at 12:09:47PM +0200, Paolo Bonzini wrote:
>>>>>> Il 16/09/2013 11:51, Fam Zheng ha scritto:
>>>>>>> On Mon, 09/16 11:44, Paolo Bonzini wrote:
>>>>>>>> Il 16/09/2013 10:59, Daniel P. Berrange ha scritto:
>>>>>>>>>> The init function of dynamic module is no longer with
>>>>>>>>>> __attribute__((constructor)) as static linked version, and need to be
>>>>>>>>>> explicitly called once loaded. The function name is mangled with per
>>>>>>>>>> configure fingerprint as:
>>>>>>>>>>
>>>>>>>>>>      init_$(date +%s$$$RANDOM)
>>>>>>>> Does this work for a module that calls module_init multiple times?
>>>>>>> Why should a module calls module_init, instead of the main function?
>>>>>> I think you mean "why should a module calls register_module_init", and I
>>>>>> agree that with this patch a module will not call register_module_init.
>>>>>>
>>>>>> But a module is still using the module_init macro.
>>>>>>
>>>>>> With this patch, a module will not be able to use the module_init macro
>>>>>> twice.  I am not sure this is an acceptable limitation, especially if we
>>>>>> do not have a dependency system within modules and/or load them with
>>>>>> G_MODULE_LOCAL/RTLD_LOCAL.
>>>>> Why would a module ever want to use the module_init macro twice ?
>>>> Because our coding standard is to have each source file do its own
>>>> one-time initialization, using static functions and an invocation of
>>>> module_init per source file.
>>> Is there ever a case where two source files, each using module_init
>>> will be compiled into the same .so loadable module. Looking at the
>>> uses of block_init(), I don't see any obvious candidates for trouble,
>>> all uses look like they'd be going into separate .so files.
>> Without inter-module exports, all of SPICE probably would have to be in
>> a single .so file.  This includes spice-qemu-char.c and
>> hw/display/qxl.c, both of which use type_init.
>>
>> If we use G_MODULE_GLOBAL as a primitive system for intermodule exports,
>> then indeed this is a much smaller problem, but then we need a
>> dependency system.  But I'm almost sure that Windows and maybe Darwin
>> lack support for the equivalent of G_MODULE_GLOBAL.
>>
> An idea for single .so file:
>      - before loads a .so, an empty initializer list is created.
>      - module_init adds a __attribute__((constructor)) function, which appends
>        its real initializer to the initializer list. So this function is
>        automatically called after dlopen().
>      - make init_$(date +%s$$$RANDOM) a dummy symbol.
>      - module_load first checks the presense of the symbol, if yes, call the
>        functions in the initializer list. Else clean up and unload .so.
>
> Does this enable multiple calls of module_init()?
>
I like this way since it keeps the old init behavior which delayed the 
work with a list.

> OTOH. As for multiple spice modules, is it possible to solve it by having a
> spice-common.o and link all spice modules to it, to share code?
>
> Fam
>

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

[Paolo Bonzini]

Il 17/09/2013 07:40, Richard Henderson ha scritto:
> On 09/16/2013 06:29 PM, Fam Zheng wrote:
>>> Link spice-qemu.char.so against spice-core.so.  The DT_NEEDED entry will be
>>> recorded, and ld.so will do the right thing.
>>>
>>> Anything else sounds way too much like Not Invented Here.
>>>
>> How to do the symbol checking as above if spice-core.so is automatically loaded
>> by ld.so?
> 
> You're checking the version stamp in spice-char.so.  I'd think that's good
> enough.  No need to transitively check.
> 
>> And we will need to add $moddir to LD_LIBRARY_PATH and lose the restriction of
>> modules directory too.
> 
> Or add DT_RUN_PATH to the main executable, or even add DT_RUN_PATH to the
> module itself.  In particular, link the module with
> 
>   -Wl,--enable-new-dtags -Wl,-rpath,'$ORIGIN'
> 
> and dependencies for the module will automatically be looked for in the
> directory in which the module is found.  Which is almost certiainly the only
> thing that we want -- all modules in the same directory.

This is not portable.

It looks like it can be emulated on Windows using LoadLibraryEx with the
LOAD_WITH_ALTERED_SEARCH_PATH flag and passing an absolute path to the
shared library.

But on Mac OS X a bundle (module loaded with dlopen) cannot have a
dependency on another bundle, afaik.

Paolo

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

[Richard Henderson]

On 09/18/2013 04:45 AM, Paolo Bonzini wrote:
> But on Mac OS X a bundle (module loaded with dlopen) cannot have a
> dependency on another bundle, afaik.

The documentation for NSAddImage suggests that they can:

> NSADDIMAGE_OPTION_WITH_SEARCHING
> With this option, the image_name passed for the library and all its
> dependents is affected by the various dyld environment variables as if this
> library were linked into the program.

Note "and all its dependents".

As for controlling the search path... from C it looks like we might be
limited to the default $LD_LIBRARY_PATH, $DYLD_LIBRARY_PATH, current working
directory, $DYLD_FALLBACK_LIBRARY_PATH" path.

It would be interesting to know when dyld reads those environment variables:
at startup only, or could we control them from main?

Failing that, it appears that explicit control over search paths can be had
from Objective C, via the NSBundle class.

Something for someone who actually cares about macosx to work on.

But you're absolutely right that we can't just whack those elf parameters into
the makefile like that.


r~

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

[Paolo Bonzini]

Il 18/09/2013 16:44, Richard Henderson ha scritto:
> On 09/18/2013 04:45 AM, Paolo Bonzini wrote:
>> But on Mac OS X a bundle (module loaded with dlopen) cannot have a
>> dependency on another bundle, afaik.
> 
> The documentation for NSAddImage suggests that they can:
> 
>> NSADDIMAGE_OPTION_WITH_SEARCHING
>> With this option, the image_name passed for the library and all its
>> dependents is affected by the various dyld environment variables as if this
>> library were linked into the program.
> 
> Note "and all its dependents".

The module can have shared libraries as dependencies.  IIRC it cannot
have other modules.

But perhaps as you wrote yesterday the SPICE core can be built as a
shared library (i.e. compiled with -shared and linked to a .dylib file).

> Failing that, it appears that explicit control over search paths can be had
> from Objective C, via the NSBundle class.

Everything you can do in ObjC you can also do in C with the
corresponding CoreFoundation library, in this case CFBundle, but it
looks like "that" bundle is a different thing than a Mach-O bundle.  It
is a directory that the UI shows as if it were a single file.  WTH...

> Something for someone who actually cares about macosx to work on.

Sure, at this point making Mac OS X not support modules actually makes
sense.  ELF and Windows seem to be okay.

Paolo