[Qemu-devel] [PATCH 0/4] buildsys: Fix module build for block-iscsi.so

[Qemu-devel] [PATCH 0/4] buildsys: Fix module build for block-iscsi.so

[Fam Zheng]

We get:

    $ qemu-img
    Failed to open module: /home/fam/build/master/block-iscsi.so: undefined symbol: bitmap_set
    qemu-img: Not enough arguments
    Try 'qemu-img --help' for more information

Because since commit b03c38 (block/iscsi: speed up read for unallocated
sectors), block/iscsi.c calls utils/bitmap.c:bitmap_* functions, which is not
linked to qemu-img nor shared objects.

This series links module objects to libqemuutil.a. That requires -fPIC on the
static library objects. The first patch unifies the dependency path of
util-obj-y to libqemuutil.a by removing individual object list from
libcacard-obj-y, so we can apply -fPIC from libqemuutils.a's object specific
variables.

Also added a travis build task for --enable-modules.

Fam

Fam Zheng (4):
  Makefile: Link vscclient with libqemuutil.a and libqemustub.a
  Makefile: Compile libqemustub.a and libqemuutil.a with -fPIC
  rules.mak: Link DSO with libqemuutil.a
  .travis.yml: Add a new build target with --enable-modules

 .travis.yml        | 3 +++
 Makefile           | 1 +
 libcacard/Makefile | 9 ++-------
 rules.mak          | 2 +-
 4 files changed, 7 insertions(+), 8 deletions(-)

-- 
1.9.2

[Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Fam Zheng]

Don't include individual objects, just link to archives.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 libcacard/Makefile | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/libcacard/Makefile b/libcacard/Makefile
index 881b222..abb6eea 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -3,12 +3,7 @@ libcacard_includedir=$(includedir)/cacard
 TOOLS += vscclient$(EXESUF)
 
 # objects linked into a shared library, built with libtool with -fPIC if required
-libcacard-obj-y = $(stub-obj-y) $(libcacard-y)
-libcacard-obj-y += util/osdep.o util/cutils.o util/qemu-timer-common.o
-libcacard-obj-y += util/error.o util/qemu-error.o
-libcacard-obj-$(CONFIG_WIN32) += util/oslib-win32.o util/qemu-thread-win32.o
-libcacard-obj-$(CONFIG_POSIX) += util/oslib-posix.o util/qemu-thread-posix.o
-libcacard-obj-y += $(filter trace/%, $(util-obj-y))
+libcacard-obj-y = $(libcacard-y)
 
 libcacard-lobj-y=$(patsubst %.o,%.lo,$(libcacard-obj-y))
 
@@ -17,7 +12,7 @@ $(libcacard-obj-y): | $(libcacard-lobj-y)
 
 all: libcacard.la libcacard.pc
 
-vscclient$(EXESUF): libcacard/vscclient.o libcacard.la
+vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libqemuutil.a libqemustub.a
 	$(call LINK,$^)
 
 #########################################################################
-- 
1.9.2

Re: [Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Paolo Bonzini]

Il 23/05/2014 05:02, Fam Zheng ha scritto:
> Don't include individual objects, just link to archives.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  libcacard/Makefile | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/libcacard/Makefile b/libcacard/Makefile
> index 881b222..abb6eea 100644
> --- a/libcacard/Makefile
> +++ b/libcacard/Makefile
> @@ -3,12 +3,7 @@ libcacard_includedir=$(includedir)/cacard
>  TOOLS += vscclient$(EXESUF)
>
>  # objects linked into a shared library, built with libtool with -fPIC if required
> -libcacard-obj-y = $(stub-obj-y) $(libcacard-y)
> -libcacard-obj-y += util/osdep.o util/cutils.o util/qemu-timer-common.o
> -libcacard-obj-y += util/error.o util/qemu-error.o
> -libcacard-obj-$(CONFIG_WIN32) += util/oslib-win32.o util/qemu-thread-win32.o
> -libcacard-obj-$(CONFIG_POSIX) += util/oslib-posix.o util/qemu-thread-posix.o
> -libcacard-obj-y += $(filter trace/%, $(util-obj-y))
> +libcacard-obj-y = $(libcacard-y)
>
>  libcacard-lobj-y=$(patsubst %.o,%.lo,$(libcacard-obj-y))
>
> @@ -17,7 +12,7 @@ $(libcacard-obj-y): | $(libcacard-lobj-y)
>
>  all: libcacard.la libcacard.pc
>
> -vscclient$(EXESUF): libcacard/vscclient.o libcacard.la
> +vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libqemuutil.a libqemustub.a
>  	$(call LINK,$^)
>
>  #########################################################################
>

Michael was going to drop the dependency altogether.  Also, I am not 
sure this patch is correct because libcacard is built using libtool and 
thus should use .la files rather than .a.

Paolo

Re: [Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Michael Tokarev]

23.05.2014 12:22, Paolo Bonzini wrote:
> Il 23/05/2014 05:02, Fam Zheng ha scritto:
[]
>> -vscclient$(EXESUF): libcacard/vscclient.o libcacard.la
>> +vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libqemuutil.a libqemustub.a
>>      $(call LINK,$^)
>>
>>  #########################################################################
>>
> 
> Michael was going to drop the dependency altogether.

This didn't go anywhere -- after spending lots of time inventing glib
thread compat layer and fighting over almost moot points I gave up.
I will be applying the libcacard de-QEMUifization patches locally in
debian.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Paolo Bonzini]

Il 23/05/2014 10:26, Michael Tokarev ha scritto:
> 23.05.2014 12:22, Paolo Bonzini wrote:
>> Il 23/05/2014 05:02, Fam Zheng ha scritto:
> []
>>> -vscclient$(EXESUF): libcacard/vscclient.o libcacard.la
>>> +vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libqemuutil.a libqemustub.a
>>>      $(call LINK,$^)
>>>
>>>  #########################################################################
>>>
>>
>> Michael was going to drop the dependency altogether.
>
> This didn't go anywhere -- after spending lots of time inventing glib
> thread compat layer and fighting over almost moot points I gave up.

Sad.  I will pick up the work.

Paolo

> I will be applying the libcacard de-QEMUifization patches locally in
> debian.

Re: [Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Michael Tokarev]

23.05.2014 13:25, Paolo Bonzini wrote:
> Il 23/05/2014 10:26, Michael Tokarev ha scritto:
>> 23.05.2014 12:22, Paolo Bonzini wrote:
>>> Il 23/05/2014 05:02, Fam Zheng ha scritto:
>> []
>>>> -vscclient$(EXESUF): libcacard/vscclient.o libcacard.la
>>>> +vscclient$(EXESUF): libcacard/vscclient.o libcacard.la libqemuutil.a libqemustub.a
>>>>      $(call LINK,$^)
>>>>
>>>>  #########################################################################
>>>>
>>>
>>> Michael was going to drop the dependency altogether.
>>
>> This didn't go anywhere -- after spending lots of time inventing glib
>> thread compat layer and fighting over almost moot points I gave up.
> 
> Sad.  I will pick up the work.

Here's the last incarnation:

 http://comments.gmane.org/gmane.comp.emulators.qemu/271372

The prob was re-defining GCond to GCond* in the _thin_ compat
layer I wanted to add for _old_ glib (so that all the code uses
new glib api, because that's what people know now).  That one
is fun indeed, but it allows us to go without inventing a thick
layer which will just complicate things and add unnecessary and
qemu-specific API -- to me it is too much cost for supporting
obsolete glib thread api.

FWIW, the old glib thread api is a frozen code, so we shouldn't
expect any changes in there.  And my changes does not wrap new
api (except of adding tiny wrappers for static initializers).

I've added just the libcacard conversion changes (which are
really trivial) to debian - since in debian we don't have that
old glib anymore, and I shouldn't care there about it anymore.
I tried to make it compatible with other systems too (yet
again: using the _thin_, almost non-existing, layer), but
apparently failed, and since I don't really care I gave up.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH 1/4] Makefile: Link vscclient with libqemuutil.a and libqemustub.a

[Paolo Bonzini]

Il 23/05/2014 11:36, Michael Tokarev ha scritto:
> Here's the last incarnation:
>
>  http://comments.gmane.org/gmane.comp.emulators.qemu/271372
>
> The prob was re-defining GCond to GCond* in the _thin_ compat
> layer I wanted to add for _old_ glib (so that all the code uses
> new glib api, because that's what people know now).  That one
> is fun indeed, but it allows us to go without inventing a thick
> layer which will just complicate things and add unnecessary and
> qemu-specific API -- to me it is too much cost for supporting
> obsolete glib thread api.
>
> FWIW, the old glib thread api is a frozen code, so we shouldn't
> expect any changes in there.  And my changes does not wrap new
> api (except of adding tiny wrappers for static initializers).

I liked your approach, but at the same time there is a problem: It would 
break completely any API that includes GMutex/GCond.  GTK itself exposes 
a GMutex when you use gtk/gtk.h.

We can exploit the macro vs. inline function difference to our 
advantage, to keep the wrapper small (just 
s/GMutex/CompatGMutex/;s/GCond/CompatGCond/) and also type safe.

I'll post my version in a second.

Paolo

[Qemu-devel] [PATCH 2/4] Makefile: Compile libqemustub.a and libqemuutil.a with -fPIC

[Fam Zheng]

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

diff --git a/Makefile b/Makefile
index d830483..9b691ec 100644
--- a/Makefile
+++ b/Makefile
@@ -203,6 +203,7 @@ Makefile: $(version-obj-y) $(version-lobj-y)
 
 libqemustub.a: $(stub-obj-y)
 libqemuutil.a: $(util-obj-y) qapi-types.o qapi-visit.o
+libqemuutil.a libqemustub.a qapi-types.o qapi-visit.o: CFLAGS += -fPIC
 
 block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) NULL
 util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
-- 
1.9.2

[Qemu-devel] [PATCH 3/4] rules.mak: Link DSO with libqemuutil.a

[Fam Zheng]

Because block-iscsi.so needs bitmap_set.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 rules.mak | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rules.mak b/rules.mak
index b12d312..6c675fd 100644
--- a/rules.mak
+++ b/rules.mak
@@ -80,7 +80,7 @@ endif
 
 DSO_CFLAGS := -fPIC -DBUILD_DSO
 %$(DSOSUF): LDFLAGS += $(LDFLAGS_SHARED)
-%$(DSOSUF): %.mo libqemustub.a
+%$(DSOSUF): %.mo libqemustub.a libqemuutil.a
 	$(call LINK,$^)
 	@# Copy to build root so modules can be loaded when program started without install
 	$(if $(findstring /,$@),$(call quiet-command,cp $@ $(subst /,-,$@), "  CP    $(subst /,-,$@)"))
-- 
1.9.2

[Qemu-devel] [PATCH 4/4] .travis.yml: Add a new build target with --enable-modules

[Fam Zheng]

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

diff --git a/.travis.yml b/.travis.yml
index 04da973..972c49a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -79,3 +79,6 @@ matrix:
           EXTRA_PKGS="liblttng-ust-dev liburcu-dev"
           EXTRA_CONFIG="--enable-trace-backend=ust"
       compiler: gcc
+    - env: TARGETS=i386-softmmu,x86_64-softmmu
+           EXTRA_CONFIG="--enable-modules"
+      compiler: gcc
-- 
1.9.2

Re: [Qemu-devel] [PATCH 0/4] buildsys: Fix module build for block-iscsi.so

[Michael Tokarev]

23.05.2014 07:02, Fam Zheng wrote:
> We get:
> 
>     $ qemu-img
>     Failed to open module: /home/fam/build/master/block-iscsi.so: undefined symbol: bitmap_set
>     qemu-img: Not enough arguments
>     Try 'qemu-img --help' for more information
> 
> Because since commit b03c38 (block/iscsi: speed up read for unallocated
> sectors), block/iscsi.c calls utils/bitmap.c:bitmap_* functions, which is not
> linked to qemu-img nor shared objects.

Heh. This is a very fun situation.

For the first time I've had it with postfix in about 2002,
when I tried to make postfix modular, and it didn't work,
because modules sometimes used symbols which are not present
in all executables who used the modules.

The solution is not link modules with missing symbols.  The
solution is to make libpostfix.so with all ths support functions,
and link all programs to it instead of using all support funcs
statically.

This is what has been done later in debian (they used simpler
approach, building 3 libs instead of one, because postfix had
3 utility libraries not 1).

Ofcourse in all cases, the soname included complete version
string, and the library was shipped in the main postfix
binary package, to avoid versioning problems.


Linking utility libraries into modules is dangerous, because
we may end up with having 2 definitions of the same symbol inside
one executable.  If that's a code part it's fine, -- both come
from the same build so the definition is the same.  But it that's
some global data section, we may have really interesting effects.
When one part of executable modifies one of the 2 global data
sections, and another part deals with another section.  Or
even more interesting -- when the same function first initializes
one mutex (in main executable), but later, after loading module,
will work with another, just loaded, incarnation of the same
mutex.

The chance to have such a clash is small.  But once we'll hit
it, it will be _extremly_ difficult to debug situation.  And
ofcourse it'll be a smoking gun, -- we may actually have it
soon, but not noticing before much later, after some unrelated
commit...

So, I'd say, please do not follow this route.. ;)

Thanks,

/mjt

> This series links module objects to libqemuutil.a. That requires -fPIC on the
> static library objects. The first patch unifies the dependency path of
> util-obj-y to libqemuutil.a by removing individual object list from
> libcacard-obj-y, so we can apply -fPIC from libqemuutils.a's object specific
> variables.
> 
> Also added a travis build task for --enable-modules.
> 
> Fam
> 
> Fam Zheng (4):
>   Makefile: Link vscclient with libqemuutil.a and libqemustub.a
>   Makefile: Compile libqemustub.a and libqemuutil.a with -fPIC
>   rules.mak: Link DSO with libqemuutil.a
>   .travis.yml: Add a new build target with --enable-modules
> 
>  .travis.yml        | 3 +++
>  Makefile           | 1 +
>  libcacard/Makefile | 9 ++-------
>  rules.mak          | 2 +-
>  4 files changed, 7 insertions(+), 8 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] buildsys: Fix module build for block-iscsi.so

[Paolo Bonzini]

Il 23/05/2014 11:59, Michael Tokarev ha scritto:
> 23.05.2014 07:02, Fam Zheng wrote:
>> We get:
>>
>>     $ qemu-img
>>     Failed to open module: /home/fam/build/master/block-iscsi.so: undefined symbol: bitmap_set
>>     qemu-img: Not enough arguments
>>     Try 'qemu-img --help' for more information
>>
>> Because since commit b03c38 (block/iscsi: speed up read for unallocated
>> sectors), block/iscsi.c calls utils/bitmap.c:bitmap_* functions, which is not
>> linked to qemu-img nor shared objects.
>
> Heh. This is a very fun situation.
>
> For the first time I've had it with postfix in about 2002,
> when I tried to make postfix modular, and it didn't work,
> because modules sometimes used symbols which are not present
> in all executables who used the modules.
>
> The solution is not link modules with missing symbols.  The
> solution is to make libpostfix.so with all ths support functions,
> and link all programs to it instead of using all support funcs
> statically.

Another solution is to turn libqemuutil.a into a libtool convenience 
library, which is a portable way to wrap it with 
--whole-archive/--no-whole-archive.  This will make libtool mandatory 
for compilation of QEMU.

Paolo

Re: [Qemu-devel] [PATCH 0/4] buildsys: Fix module build for block-iscsi.so

[Paolo Bonzini]

Il 23/05/2014 05:02, Fam Zheng ha scritto:
> We get:
>
>     $ qemu-img
>     Failed to open module: /home/fam/build/master/block-iscsi.so: undefined symbol: bitmap_set
>     qemu-img: Not enough arguments
>     Try 'qemu-img --help' for more information
>
> Because since commit b03c38 (block/iscsi: speed up read for unallocated
> sectors), block/iscsi.c calls utils/bitmap.c:bitmap_* functions, which is not
> linked to qemu-img nor shared objects.
>
> This series links module objects to libqemuutil.a. That requires -fPIC on the
> static library objects. The first patch unifies the dependency path of
> util-obj-y to libqemuutil.a by removing individual object list from
> libcacard-obj-y, so we can apply -fPIC from libqemuutils.a's object specific
> variables.
>
> Also added a travis build task for --enable-modules.
>
> Fam
>
> Fam Zheng (4):
>   Makefile: Link vscclient with libqemuutil.a and libqemustub.a
>   Makefile: Compile libqemustub.a and libqemuutil.a with -fPIC
>   rules.mak: Link DSO with libqemuutil.a
>   .travis.yml: Add a new build target with --enable-modules
>
>  .travis.yml        | 3 +++
>  Makefile           | 1 +
>  libcacard/Makefile | 9 ++-------
>  rules.mak          | 2 +-
>  4 files changed, 7 insertions(+), 8 deletions(-)
>

Applying patch 4.

Paolo