subject:"\[libvirt\] \[PATCH\] util\: avoid symbol clash between json libraries"

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-05 Thread Peter Krempa

On Tue, Jul 31, 2018 at 15:55:28 +0100, Daniel Berrange wrote:
> The jansson and json-glib libraries both export symbols with a json_
> name prefix and json_object_iter_next() clashes between them.
> 
> Unfortunately json_glib is linked in by GTK, so any app using GTK and
> libvirt will get a clash, resulting in SEGV. This also affects the NSS
> module provided by libvirt
> 
> Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
> flag which allows us to hide the symbols from the application that loads
> libvirt or the NSS module.
> 
> Some preprocessor black magic and wrapper functions are used to redirect
> calls into the dlopen resolved symbols.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  libvirt.spec.in  |   2 +
>  src/Makefile.am  |   3 +
>  src/util/Makefile.inc.am |   3 +-
>  src/util/virjson.c   |   9 +-
>  src/util/virjsoncompat.c | 253 +++
>  src/util/virjsoncompat.h |  86 +
>  6 files changed, 354 insertions(+), 2 deletions(-)
>  create mode 100644 src/util/virjsoncompat.c
>  create mode 100644 src/util/virjsoncompat.h

[...]

> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 01a387b2f7..5bab662cd3 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
>  
>  
>  #if WITH_JANSSON
> -# include 
> +
> +# include "virjsoncompat.h"
>  
>  static virJSONValuePtr
>  virJSONValueFromJansson(json_t *json)
> @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
>  size_t flags = JSON_REJECT_DUPLICATES |
> JSON_DECODE_ANY;
>  
> +if (virJSONInitialize() < 0)
> +return NULL;
> +
>  if (!(json = json_loads(jsonstring, flags, ))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("failed to parse JSON %d:%d: %s"),
> @@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
>  json_t *json;
>  char *str = NULL;
>  
> +if (virJSONInitialize() < 0)
> +return NULL;
> +
>  if (pretty)
>  flags |= JSON_INDENT(2);
>  else

[Note that some mails did not make it to the list so I don't know if
this issue was raised.]

If we initialize this when doing JSON operations and the initialization
fails we'll end up with libvirtd in a crippled state not able to use
anything in qemu which is in my opinion VERY bad.

In addition it also kills all running VMs when reconnecting since
monitor will just not work.

I think libvirtd should terminate right away if we can't initialize this
shim rather than execute some other code.

Additionally, why is this even necessary for libvirtd? Not that it would
ever link with any clashing libraries.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-05 Thread Ján Tomko


On Tue, Jul 31, 2018 at 03:55:28PM +0100, Daniel P. Berrangé wrote:

The jansson and json-glib libraries both export symbols with a json_
name prefix and json_object_iter_next() clashes between them.

Unfortunately json_glib is linked in by GTK, so any app using GTK and


json-glib


libvirt will get a clash, resulting in SEGV. This also affects the NSS
module provided by libvirt

Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
flag which allows us to hide the symbols from the application that loads
libvirt or the NSS module.

Some preprocessor black magic and wrapper functions are used to redirect
calls into the dlopen resolved symbols.

Signed-off-by: Daniel P. Berrangé 
---
libvirt.spec.in  |   2 +
src/Makefile.am  |   3 +
src/util/Makefile.inc.am |   3 +-
src/util/virjson.c   |   9 +-
src/util/virjsoncompat.c | 253 +++
src/util/virjsoncompat.h |  86 +
6 files changed, 354 insertions(+), 2 deletions(-)
create mode 100644 src/util/virjsoncompat.c
create mode 100644 src/util/virjsoncompat.h

diff --git a/src/util/virjsoncompat.c b/src/util/virjsoncompat.c
new file mode 100644
index 00..c317e50c32
--- /dev/null
+++ b/src/util/virjsoncompat.c
@@ -0,0 +1,253 @@
+/*
+ * virjsoncompat.c: JSON object parsing/formatting
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+#include 
+
+#include "virthread.h"
+#include "virerror.h"



+#define VIR_JSON_COMPAT_IMPL
+#include "virjsoncompat.h"


virjsoncompat.h includes jansson.h unconditionally, so this fails to
compile on a machine without jansson-devel:
In file included from util/virjsoncompat.c:27:
util/virjsoncompat.h:56:10: fatal error: jansson.h: No such file or directory
#include 
 ^~~


+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+#if WITH_JANSSON
+
+#include 
+
+json_t *(*json_array_ptr)(void);
+int (*json_array_append_new_ptr)(json_t *array, json_t *value);
+json_t *(*json_array_get_ptr)(const json_t *array, size_t index);
+size_t (*json_array_size_ptr)(const json_t *array);
+void (*json_delete_ptr)(json_t *json);
+char *(*json_dumps_ptr)(const json_t *json, size_t flags);
+json_t *(*json_false_ptr)(void);
+json_t *(*json_integer_ptr)(json_int_t value);
+json_int_t (*json_integer_value_ptr)(const json_t *integer);
+json_t *(*json_loads_ptr)(const char *input, size_t flags, json_error_t 
*error);
+json_t *(*json_null_ptr)(void);
+json_t *(*json_object_ptr)(void);
+void *(*json_object_iter_ptr)(json_t *object);
+const char *(*json_object_iter_key_ptr)(void *iter);
+void *(*json_object_iter_next_ptr)(json_t *object, void *iter);
+json_t *(*json_object_iter_value_ptr)(void *iter);
+void *(*json_object_key_to_iter_ptr)(const char *key);
+int (*json_object_set_new_ptr)(json_t *object, const char *key, json_t *value);
+json_t *(*json_real_ptr)(double value);
+double (*json_real_value_ptr)(const json_t *real);
+json_t *(*json_string_ptr)(const char *value);
+const char *(*json_string_value_ptr)(const json_t *string);
+json_t *(*json_true_ptr)(void);
+
+
+static int virJSONJanssonOnceInit(void)
+{
+void *handle = dlopen("libjansson.so.4", 
RTLD_LAZY|RTLD_LOCAL|RTLD_DEEPBIND|RTLD_NODELETE);
+if (!handle) {
+virReportError(VIR_ERR_NO_SUPPORT,
+   _("libjansson.so.4 JSON library not available: %s"), 
dlerror());
+return -1;
+}
+
+#define LOAD(name) \
+do { \
+if (!(name ## _ptr = dlsym(handle, #name))) {  \
+virReportError(VIR_ERR_NO_SUPPORT,  \
+   _("missing symbol '%s' in libjansson.so.4: %s"), 
#name, dlerror()); \
+goto error; \


If you do
 return -1;
you can drop the error label.


+} \
+fprintf(stderr, "Resolve %s to %p\n", #name, name ## _ptr); \
+} while (0)
+
+LOAD(json_array);
+LOAD(json_array_append_new);
+LOAD(json_array_get);
+LOAD(json_array_size);
+LOAD(json_delete);
+LOAD(json_dumps);
+LOAD(json_false);
+LOAD(json_integer);
+LOAD(json_integer_value);
+LOAD(json_loads);
+LOAD(json_null);
+LOAD(json_object);
+LOAD(json_object_iter);
+LOAD(json_object_iter_key);
+LOAD(json_object_iter_next);

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-05 Thread Eric Blake


On 07/31/2018 09:55 AM, Daniel P. Berrangé wrote:

The jansson and json-glib libraries both export symbols with a json_
name prefix and json_object_iter_next() clashes between them.

Unfortunately json_glib is linked in by GTK, so any app using GTK and
libvirt will get a clash, resulting in SEGV. This also affects the NSS
module provided by libvirt

Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
flag which allows us to hide the symbols from the application that loads
libvirt or the NSS module.

Some preprocessor black magic and wrapper functions are used to redirect
calls into the dlopen resolved symbols.


Would using dlmopen() instead of dlopen() make this task any easier?

Otherwise, this looks reasonable to me, and is preferable to Jan's 
proposal to revert jansson support.



+++ b/src/util/virjsoncompat.c



+
+static int virJSONJanssonOnceInit(void)
+{
+void *handle = dlopen("libjansson.so.4", 
RTLD_LAZY|RTLD_LOCAL|RTLD_DEEPBIND|RTLD_NODELETE);


RTLD_DEEPBIND might be specific to glibc; is this going to cause any 
compilation issues on BSD machines?




+
+VIR_ONCE_GLOBAL_INIT(virJSONJansson);
+
+int virJSONInitialize(void) {
+return virJSONJanssonInitialize();
+}
+
+json_t *json_array_impl(void)
+{
+return json_array_ptr();
+}
+
+
+int json_array_append_new_impl(json_t *array, json_t *value)
+{
+return json_array_append_new_ptr(array, value);
+}


Would it be possible with __typeof__ to write a macro to make this 
forwarding more compact (one line per stem, instead of open-coding each 
renamed function)?  Looking something like:


#define FORWARD(name, params, args) \
  __typeof__(name # _impl) name # _impl params { \
return name # _ptr args; \
  }

FORWARD(json_array, (void), ())
FORWARD(json_array_append_new, (json_t * array, json_t *value),
 (array, value))

(hmm, that's still a bit verbose; I'm not sure if the preprocessor could 
be cajoled into even less repetition of parameter names)


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-05 Thread Andrea Bolognani

On Tue, 2018-07-31 at 15:55 +0100, Daniel P. Berrangé wrote:
[...]
> +# We dlopen() it so need an explicit dep
> +Requires: libjansson.so.4()(64bit)

Wouldn't requiring jansson be better here? I don't think many
people are running libvirt on 32-bit machines these days, but
the (64bit) part still looks kinda weird.

> @@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
>   $(NULL)
>  libvirt_util_la_LIBADD = \
>   $(CAPNG_LIBS) \
> - $(JANSSON_LIBS) \

You missed a couple of instances of $(JANSSON_LIBS), notably the
one used to link the NSS plugin against it ;)

> @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
>  size_t flags = JSON_REJECT_DUPLICATES |
> JSON_DECODE_ANY;
>  
> +if (virJSONInitialize() < 0)
> +return NULL;

Shouldn't we rather virReportError() here? Or does it not matter
since we do that inside virJSONInitialize() already?

[...]
> +#define LOAD(name) \
> +do { \
> +if (!(name ## _ptr = dlsym(handle, #name))) {  \
> +virReportError(VIR_ERR_NO_SUPPORT,  \
> +   _("missing symbol '%s' in libjansson.so.4: %s"), 
> #name, dlerror()); \
> +goto error; \
> +} \

The lines above trigger a syntax-check failure; there are other
issues as well, please make sure you address all of them.

> +fprintf(stderr, "Resolve %s to %p\n", #name, name ## _ptr); \

I guess the fprintf() is a leftover from development: please drop
it to avoid stuff like

  $ ping debian
  Resolve json_array to 0x7fdcf77b2fa0
  Resolve json_array_append_new to 0x7fdcf77b3e60
  ...
  Resolve json_string_value to 0x7fdcf77b3200
  Resolve json_true to 0x7fdcf77b36c0
  PING debian (192.168.124.124) 56(84) bytes of data.
  64 bytes from 192.168.124.124 (192.168.124.124): icmp_seq=1 ttl=64 time=0.164 
ms

[...]
> +int virJSONInitialize(void) {
> +return virJSONJanssonInitialize();
> +}

Please format this as

  int
  virJSONInitialize(void) {
  ...

Same for all the stubs below.


Everything else looks pretty much as sane as a crazy hack like this
could possibly ever look :) so with the above addressed

  Reviewed-by: Andrea Bolognani 

with the caveats that you should probably wait for at least another
ACK before pushing it, and additionally we should *really* bump up
the release date and let this sit on master for more than a couple
of days...

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-05 Thread no-reply

Hi,

This series was run against 'syntax-check' test by patchew.org, which failed, 
please find the details below:

Type: series
Message-id: 20180731145528.9845-1-berra...@redhat.com
Subject: [libvirt] [PATCH] util: avoid symbol clash between json libraries

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
time bash -c './autogen.sh && make syntax-check'
=== TEST SCRIPT END ===

Updating bcb55ab053bc79561b55d0394490f4b64e0f2d01
>From https://github.com/patchew-project/libvirt
   e9024b0cec..7bda210e6d  master -> master
 * [new tag]   patchew/20180731145528.9845-1-berra...@redhat.com -> 
patchew/20180731145528.9845-1-berra...@redhat.com
Switched to a new branch 'test'
d9528ced11 util: avoid symbol clash between json libraries

=== OUTPUT BEGIN ===
Updating submodules...
Submodule 'gnulib' (https://git.savannah.gnu.org/git/gnulib.git/) registered 
for path '.gnulib'
Submodule 'keycodemapdb' (https://gitlab.com/keycodemap/keycodemapdb.git) 
registered for path 'src/keycodemapdb'
Cloning into '/var/tmp/patchew-tester-tmp-jch49p_t/src/.gnulib'...
Cloning into '/var/tmp/patchew-tester-tmp-jch49p_t/src/src/keycodemapdb'...
Submodule path '.gnulib': checked out '68df637b5f1b5c10370f6981d2a43a5cf74368df'
Submodule path 'src/keycodemapdb': checked out 
'16e5b0787687d8904dad2c026107409eb9bfcb95'
Running bootstrap...
./bootstrap: Bootstrapping from checked-out libvirt sources...
./bootstrap: consider installing git-merge-changelog from gnulib
./bootstrap: getting gnulib files...
running: libtoolize --install --copy
libtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
libtoolize: copying file 'build-aux/config.guess'
libtoolize: copying file 'build-aux/config.sub'
libtoolize: copying file 'build-aux/install-sh'
libtoolize: copying file 'build-aux/ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'.
libtoolize: copying file 'm4/libtool.m4'
libtoolize: copying file 'm4/ltoptions.m4'
libtoolize: copying file 'm4/ltsugar.m4'
libtoolize: copying file 'm4/ltversion.m4'
libtoolize: copying file 'm4/lt~obsolete.m4'
./bootstrap: .gnulib/gnulib-tool--no-changelog   --aux-dir=build-aux   
--doc-base=doc   --lib=libgnu   --m4-base=m4/   --source-base=gnulib/lib/   
--tests-base=gnulib/tests   --local-dir=gnulib/local--lgpl=2 --with-tests 
--makefile-name=gnulib.mk --avoid=pt_chown --avoid=lock-tests   --libtool 
--import ...
Module list with included dependencies (indented):
absolute-header
  accept
accept-tests
alloca
alloca-opt
alloca-opt-tests
allocator
  areadlink
areadlink-tests
arpa_inet
arpa_inet-tests
assure
  autobuild
  base64
base64-tests
binary-io
binary-io-tests
  bind
bind-tests
  bitrotate
bitrotate-tests
btowc
btowc-tests
builtin-expect
  byteswap
byteswap-tests
  c-ctype
c-ctype-tests
  c-strcase
c-strcase-tests
  c-strcasestr
c-strcasestr-tests
  calloc-posix
  canonicalize-lgpl
canonicalize-lgpl-tests
careadlinkat
  chown
chown-tests
  clock-time
cloexec
cloexec-tests
  close
close-tests
  configmake
  connect
connect-tests
  count-leading-zeros
count-leading-zeros-tests
  count-one-bits
count-one-bits-tests
ctype
ctype-tests
  dirname-lgpl
dosname
double-slash-root
dup
dup-tests
dup2
dup2-tests
  environ
environ-tests
errno
errno-tests
error
  execinfo
exitfail
extensions
extern-inline
fatal-signal
  fclose
fclose-tests
  fcntl
  fcntl-h
fcntl-h-tests
fcntl-tests
fd-hook
  fdatasync
fdatasync-tests
fdopen
fdopen-tests
fflush
fflush-tests
  ffs
ffs-tests
  ffsl
ffsl-tests
fgetc-tests
filename
flexmember
float
float-tests
  fnmatch
fnmatch-tests
fpieee
fpucw
fpurge
fpurge-tests
fputc-tests
fread-tests
freading
freading-tests
fseek
fseek-tests
fseeko
fseeko-tests
fstat
fstat-tests
  fsync
fsync-tests
ftell
ftell-tests
ftello
ftello-tests
ftruncate
ftruncate-tests
  func
func-tests
fwrite-tests
  getaddrinfo
getaddrinfo-tests
  getcwd-lgpl
getcwd-lgpl-tests
getdelim
getdelim-tests
getdtablesize
getdtablesize-tests
getgroups
getgroups-tests
  gethostname
gethostname-tests
getline
getline-tests
  getopt-posix
getopt-posix-tests
getpagesize
  getpass
  getpeername
getpeername-tests
getprogname
getprogname-tests
  getsockname
getsockname-tests
getsockopt
getsockopt-tests
gettext-h
  gettimeofday
gettimeofday-tests
getugroups
  gitlog-to-changelog
  gnumakefile
grantpt
grantpt-tests
hard-locale
havelib
host-cpu-c-abi
hostent
  ignore-value
ignore-v

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-08-01 Thread Daniel P . Berrangé

On Wed, Aug 01, 2018 at 09:31:49AM +0200, Peter Krempa wrote:
> On Tue, Jul 31, 2018 at 15:55:28 +0100, Daniel Berrange wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> > libvirt will get a clash, resulting in SEGV. This also affects the NSS
> > module provided by libvirt
> > 
> > Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
> > flag which allows us to hide the symbols from the application that loads
> > libvirt or the NSS module.
> > 
> > Some preprocessor black magic and wrapper functions are used to redirect
> > calls into the dlopen resolved symbols.
> > 
> > Signed-off-by: Daniel P. Berrangé 
> > ---
> >  libvirt.spec.in  |   2 +
> >  src/Makefile.am  |   3 +
> >  src/util/Makefile.inc.am |   3 +-
> >  src/util/virjson.c   |   9 +-
> >  src/util/virjsoncompat.c | 253 +++
> >  src/util/virjsoncompat.h |  86 +
> >  6 files changed, 354 insertions(+), 2 deletions(-)
> >  create mode 100644 src/util/virjsoncompat.c
> >  create mode 100644 src/util/virjsoncompat.h
> 
> [...]
> 
> > diff --git a/src/util/virjson.c b/src/util/virjson.c
> > index 01a387b2f7..5bab662cd3 100644
> > --- a/src/util/virjson.c
> > +++ b/src/util/virjson.c
> > @@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
> >  
> >  
> >  #if WITH_JANSSON
> > -# include 
> > +
> > +# include "virjsoncompat.h"
> >  
> >  static virJSONValuePtr
> >  virJSONValueFromJansson(json_t *json)
> > @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
> >  size_t flags = JSON_REJECT_DUPLICATES |
> > JSON_DECODE_ANY;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> > +
> >  if (!(json = json_loads(jsonstring, flags, ))) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("failed to parse JSON %d:%d: %s"),
> > @@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
> >  json_t *json;
> >  char *str = NULL;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> > +
> >  if (pretty)
> >  flags |= JSON_INDENT(2);
> >  else
> 
> [Note that some mails did not make it to the list so I don't know if
> this issue was raised.]
> 
> If we initialize this when doing JSON operations and the initialization
> fails we'll end up with libvirtd in a crippled state not able to use
> anything in qemu which is in my opinion VERY bad.
> 
> In addition it also kills all running VMs when reconnecting since
> monitor will just not work.
> 
> I think libvirtd should terminate right away if we can't initialize this
> shim rather than execute some other code.

We can do that by calling virJSONInitialize from libvirtd main. It should
never fail unless someone instaled libvirtd without jansson being present,
or if jansson broke its API.

> Additionally, why is this even necessary for libvirtd? Not that it would
> ever link with any clashing libraries.

It may not currently be neccessary for libvirtd, but I can't guarantee
that's always going to be the case. We link to so many libraries, we can
be surprised at anytime with something pulling in a clashing json impl.
The json-c library also includes symbols with a json_* prefix in their
name, so that's a 3d json impl which might clash.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-07-31 Thread Daniel P . Berrangé

On Tue, Jul 31, 2018 at 11:28:02AM -0500, Eric Blake wrote:
> On 07/31/2018 09:55 AM, Daniel P. Berrangé wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> > libvirt will get a clash, resulting in SEGV. This also affects the NSS
> > module provided by libvirt
> > 
> > Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
> > flag which allows us to hide the symbols from the application that loads
> > libvirt or the NSS module.
> > 
> > Some preprocessor black magic and wrapper functions are used to redirect
> > calls into the dlopen resolved symbols.
> 
> Would using dlmopen() instead of dlopen() make this task any easier?

Not available on *BSD

> 
> Otherwise, this looks reasonable to me, and is preferable to Jan's proposal
> to revert jansson support.
> 
> > +++ b/src/util/virjsoncompat.c
> 
> > +
> > +static int virJSONJanssonOnceInit(void)
> > +{
> > +void *handle = dlopen("libjansson.so.4", 
> > RTLD_LAZY|RTLD_LOCAL|RTLD_DEEPBIND|RTLD_NODELETE);
> 
> RTLD_DEEPBIND might be specific to glibc; is this going to cause any
> compilation issues on BSD machines?

Yeah it broke BSD, so removed in v2.

> > +int json_array_append_new_impl(json_t *array, json_t *value)
> > +{
> > +return json_array_append_new_ptr(array, value);
> > +}
> 
> Would it be possible with __typeof__ to write a macro to make this
> forwarding more compact (one line per stem, instead of open-coding each
> renamed function)?  Looking something like:
> 
> #define FORWARD(name, params, args) \
>   __typeof__(name # _impl) name # _impl params { \
> return name # _ptr args; \
>   }
> 
> FORWARD(json_array, (void), ())
> FORWARD(json_array_append_new, (json_t * array, json_t *value),
>  (array, value))
> 
> (hmm, that's still a bit verbose; I'm not sure if the preprocessor could be
> cajoled into even less repetition of parameter names)

I'm not too fussed about the verbosity as this is write-once code (i hope).

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-07-31 Thread Daniel P . Berrangé

On Tue, Jul 31, 2018 at 06:07:20PM +0200, Ján Tomko wrote:
> On Tue, Jul 31, 2018 at 03:55:28PM +0100, Daniel P. Berrangé wrote:
> > The jansson and json-glib libraries both export symbols with a json_
> > name prefix and json_object_iter_next() clashes between them.
> > 
> > Unfortunately json_glib is linked in by GTK, so any app using GTK and
> 
> json-glib

Not fixed in v2, but fixed in my local copy now.

> > +#include 
> > +
> > +#include "virthread.h"
> > +#include "virerror.h"
> 
> > +#define VIR_JSON_COMPAT_IMPL
> > +#include "virjsoncompat.h"
> 
> virjsoncompat.h includes jansson.h unconditionally, so this fails to
> compile on a machine without jansson-devel:
> In file included from util/virjsoncompat.c:27:
> util/virjsoncompat.h:56:10: fatal error: jansson.h: No such file or directory
> #include 
>  ^~~

I've rearranged things and tested a mingw build now so this should
be ok in v2.

> > +#define LOAD(name) \
> > +do { \
> > +if (!(name ## _ptr = dlsym(handle, #name))) {  \
> > +virReportError(VIR_ERR_NO_SUPPORT,  \
> > +   _("missing symbol '%s' in libjansson.so.4: 
> > %s"), #name, dlerror()); \
> > +goto error; \
> 
> If you do
>  return -1;
> you can drop the error label.

Yes ok




Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-07-31 Thread Daniel P . Berrangé

On Tue, Jul 31, 2018 at 05:57:21PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-07-31 at 15:55 +0100, Daniel P. Berrangé wrote:
> [...]
> > +# We dlopen() it so need an explicit dep
> > +Requires: libjansson.so.4()(64bit)
> 
> Wouldn't requiring jansson be better here? I don't think many
> people are running libvirt on 32-bit machines these days, but
> the (64bit) part still looks kinda weird.

Yes, ok.

> > @@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
> > $(NULL)
> >  libvirt_util_la_LIBADD = \
> > $(CAPNG_LIBS) \
> > -   $(JANSSON_LIBS) \
> 
> You missed a couple of instances of $(JANSSON_LIBS), notably the
> one used to link the NSS plugin against it ;)
> 
> > @@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
> >  size_t flags = JSON_REJECT_DUPLICATES |
> > JSON_DECODE_ANY;
> >  
> > +if (virJSONInitialize() < 0)
> > +return NULL;
> 
> Shouldn't we rather virReportError() here? Or does it not matter
> since we do that inside virJSONInitialize() already?

That method is already reporting errors when needed.

> The lines above trigger a syntax-check failure; there are other
> issues as well, please make sure you address all of them.

Yeah done in v2.

> 
> > +fprintf(stderr, "Resolve %s to %p\n", #name, name ## _ptr); \
> 
> I guess the fprintf() is a leftover from development: please drop
> it to avoid stuff like

Opps.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] util: avoid symbol clash between json libraries

2018-07-31 Thread Daniel P . Berrangé

The jansson and json-glib libraries both export symbols with a json_
name prefix and json_object_iter_next() clashes between them.

Unfortunately json_glib is linked in by GTK, so any app using GTK and
libvirt will get a clash, resulting in SEGV. This also affects the NSS
module provided by libvirt

Instead of directly linking to jansson, use dlopen() with the RTLD_LOCAL
flag which allows us to hide the symbols from the application that loads
libvirt or the NSS module.

Some preprocessor black magic and wrapper functions are used to redirect
calls into the dlopen resolved symbols.

Signed-off-by: Daniel P. Berrangé 
---
 libvirt.spec.in  |   2 +
 src/Makefile.am  |   3 +
 src/util/Makefile.inc.am |   3 +-
 src/util/virjson.c   |   9 +-
 src/util/virjsoncompat.c | 253 +++
 src/util/virjsoncompat.h |  86 +
 6 files changed, 354 insertions(+), 2 deletions(-)
 create mode 100644 src/util/virjsoncompat.c
 create mode 100644 src/util/virjsoncompat.h

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 4113579e47..cfe7ab8a09 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -898,6 +898,8 @@ Requires: ncurses
 Requires: gettext
 # Needed by virt-pki-validate script.
 Requires: gnutls-utils
+# We dlopen() it so need an explicit dep
+Requires: libjansson.so.4()(64bit)
 %if %{with_bash_completion}
 Requires: %{name}-bash-completion = %{version}-%{release}
 %endif
diff --git a/src/Makefile.am b/src/Makefile.am
index 83263e69e5..59ae7a2e79 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -690,6 +690,7 @@ libvirt_setuid_rpc_client_la_SOURCES = \
util/virhashcode.c \
util/virhostcpu.c \
util/virjson.c \
+   util/virjsoncompat.c \
util/virlog.c \
util/virobject.c \
util/virpidfile.c \
@@ -961,6 +962,8 @@ libvirt_nss_la_SOURCES = \
util/virhashcode.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkmod.c \
util/virkmod.h \
util/virlease.c \
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index 71b2b93c2d..8ef9ee1dfa 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -86,6 +86,8 @@ UTIL_SOURCES = \
util/viriscsi.h \
util/virjson.c \
util/virjson.h \
+   util/virjsoncompat.c \
+   util/virjsoncompat.h \
util/virkeycode.c \
util/virkeycode.h \
util/virkeyfile.c \
@@ -264,7 +266,6 @@ libvirt_util_la_CFLAGS = \
$(NULL)
 libvirt_util_la_LIBADD = \
$(CAPNG_LIBS) \
-   $(JANSSON_LIBS) \
$(LIBNL_LIBS) \
$(THREAD_LIBS) \
$(AUDIT_LIBS) \
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 01a387b2f7..5bab662cd3 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1437,7 +1437,8 @@ virJSONValueCopy(const virJSONValue *in)
 
 
 #if WITH_JANSSON
-# include 
+
+# include "virjsoncompat.h"
 
 static virJSONValuePtr
 virJSONValueFromJansson(json_t *json)
@@ -1524,6 +1525,9 @@ virJSONValueFromString(const char *jsonstring)
 size_t flags = JSON_REJECT_DUPLICATES |
JSON_DECODE_ANY;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (!(json = json_loads(jsonstring, flags, ))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to parse JSON %d:%d: %s"),
@@ -1630,6 +1634,9 @@ virJSONValueToString(virJSONValuePtr object,
 json_t *json;
 char *str = NULL;
 
+if (virJSONInitialize() < 0)
+return NULL;
+
 if (pretty)
 flags |= JSON_INDENT(2);
 else
diff --git a/src/util/virjsoncompat.c b/src/util/virjsoncompat.c
new file mode 100644
index 00..c317e50c32
--- /dev/null
+++ b/src/util/virjsoncompat.c
@@ -0,0 +1,253 @@
+/*
+ * virjsoncompat.c: JSON object parsing/formatting
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ */
+
+#include 
+
+#include "virthread.h"
+#include "virerror.h"
+#define VIR_JSON_COMPAT_IMPL
+#include "virjsoncompat.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+#if WITH_JANSSON
+
+#include 
+
+json_t *(*json_array_ptr)(void);
+int

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries

[libvirt] [PATCH] util: avoid symbol clash between json libraries

10 matches

Site Navigation

Mail list logo

Footer information