diff mbox

[2/5] vdi: don't override libuuid symbols

Message ID 1353492752-16084-3-git-send-email-stefanha@redhat.com
State New
Headers show

Commit Message

Stefan Hajnoczi Nov. 21, 2012, 10:12 a.m. UTC 
It's poor symbol hygiene to provide a global symbols that collide with a
common library like libuuid.  If QEMU links against a shared library
that depends on uuid_generate() it can end up calling our stub version
of the function.

This exact scenario happened with GlusterFS libgfapi.so, which depends
on libglusterfs.so's uuid_generate().

Scope the uuid stubs for vdi.c only and avoid affecting other shared
objects.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vdi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Stefan Weil Nov. 21, 2012, 7:25 p.m. UTC | #1 
Am 21.11.2012 11:12, schrieb Stefan Hajnoczi:
> It's poor symbol hygiene to provide a global symbols that collide with a
> common library like libuuid.  If QEMU links against a shared library
> that depends on uuid_generate() it can end up calling our stub version
> of the function.
>
> This exact scenario happened with GlusterFS libgfapi.so, which depends
> on libglusterfs.so's uuid_generate().
>
> Scope the uuid stubs for vdi.c only and avoid affecting other shared
> objects.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vdi.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>


Hi Stefan,

I'm not opposed to this patch but would like to better understand
the problem which you had with the old solution.

Does libglusterfs.so really have a uuid_generate? Why does it not
take the implementation from libuuid.so? Then QEMU could also
use libuuid.so, and there would be no problem at all.

I tried to reproduce the problem but could not find a libgfapi.so
in the Debian or Ubuntu package archives.

Regards
Stefan
Stefan Hajnoczi Nov. 22, 2012, 7:33 a.m. UTC | #2 
On Wed, Nov 21, 2012 at 08:25:24PM +0100, Stefan Weil wrote:
> Am 21.11.2012 11:12, schrieb Stefan Hajnoczi:
> >It's poor symbol hygiene to provide a global symbols that collide with a
> >common library like libuuid.  If QEMU links against a shared library
> >that depends on uuid_generate() it can end up calling our stub version
> >of the function.
> >
> >This exact scenario happened with GlusterFS libgfapi.so, which depends
> >on libglusterfs.so's uuid_generate().
> >
> >Scope the uuid stubs for vdi.c only and avoid affecting other shared
> >objects.
> >
> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >---
> >  block/vdi.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> >
> 
> 
> Hi Stefan,
> 
> I'm not opposed to this patch but would like to better understand
> the problem which you had with the old solution.
> 
> Does libglusterfs.so really have a uuid_generate? Why does it not
> take the implementation from libuuid.so? Then QEMU could also
> use libuuid.so, and there would be no problem at all.
> 
> I tried to reproduce the problem but could not find a libgfapi.so
> in the Debian or Ubuntu package archives.

libgfapi.so is only in glusterfs.git/master.  It has not yet been
available in a release.

The problem occurs when QEMU and GlusterFS are built on a system without
the libuuid.h development header.

QEMU's behavior in this situation is to build our own libuuid stubs in
block/vdi.c.  The symbols are global.

GlusterFS's behavior in this situation is to build with its own libuuid
implementation.  The symbols are global in libglusterfs.so.  The
libgfapi.so shared object links against libglusterfs.so and expects its
uuid_generate() symbol to be resolved.

The fun happens when using QEMU block/gluster.c and it calls into
libgfapi.so.  It seems QEMU's block/vdi.c libuuid stubs are bound and
GlusterFS is now calling QEMU's dummy code - uuid_generate() always
returns the zero UUID!

Both QEMU and GlusterFS should not provide global symbols for a common
library.

Stefan
diff mbox

Patch

diff --git a/block/vdi.c b/block/vdi.c
index f35b12e..c8330b7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -60,9 +60,6 @@ 
 /* TODO: move uuid emulation to some central place in QEMU. */
 #include "sysemu.h"     /* UUID_FMT */
 typedef unsigned char uuid_t[16];
-void uuid_generate(uuid_t out);
-int uuid_is_null(const uuid_t uu);
-void uuid_unparse(const uuid_t uu, char *out);
 #endif
 
 /* Code configuration options. */
@@ -124,18 +121,18 @@  void uuid_unparse(const uuid_t uu, char *out);
 #define VDI_IS_ALLOCATED(X) ((X) < VDI_DISCARDED)
 
 #if !defined(CONFIG_UUID)
-void uuid_generate(uuid_t out)
+static inline void uuid_generate(uuid_t out)
 {
     memset(out, 0, sizeof(uuid_t));
 }
 
-int uuid_is_null(const uuid_t uu)
+static inline int uuid_is_null(const uuid_t uu)
 {
     uuid_t null_uuid = { 0 };
     return memcmp(uu, null_uuid, sizeof(uuid_t)) == 0;
 }
 
-void uuid_unparse(const uuid_t uu, char *out)
+static inline void uuid_unparse(const uuid_t uu, char *out)
 {
     snprintf(out, 37, UUID_FMT,
             uu[0], uu[1], uu[2], uu[3], uu[4], uu[5], uu[6], uu[7],