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@xxxxxxxxxx>
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
