Re: [libvirt] [PATCH] util: avoid symbol clash between json libraries
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 <abologna(a)redhat.com>
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