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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux