Re: [libvirt] [PATCH 8/9] virjson: add support for Jansson

On Tue, Apr 03, 2018 at 11:39:20 +0100, Daniel Berrange wrote: > On Sat, Mar 31, 2018 at 11:13:13AM +0200, Peter Krempa wrote: > > On Thu, Mar 29, 2018 at 01:09:57 +0200, Ján Tomko wrote: > > > Check for the presence of Jansson library and prefer it to yajl > > > if possible. > > > > > > The minimum required version is 2.7. > > > > > > Internally, virJSONValue still stores numbers as strings even > > > though Jansson uses numeric variables for them. > > > > > > The configure script is particularly hideous, but will hopefully > > > go away after we stop aiming to support compiling on CentOS 6. > > > > > > Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; > > > --- > > > configure.ac | 1 + > > > m4/virt-json.m4 | 55 +++++++++++--- > > > src/util/virjson.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 264 insertions(+), 11 deletions(-) [...] > > > +static json_t * > > > +virJSONValueToJansson(virJSONValuePtr object) > > > +{ > > > + json_error_t error; > > > + json_t *ret = NULL; > > > + size_t i; > > > + > > > + switch (object->type) { > > > + case VIR_JSON_TYPE_OBJECT: > > > + ret = json_object(); > > > + if (!ret) { > > > > So this would be the second copy of a similar function. I propose that > > we replace the formatter which in this case copies everything from our > > structs into structs of janson to format it with a formatter which > > directly uses virBuffer to do so. > > Note the second copy will drop back down to 1 copy when we later > drop YAJL support, so this is not a long term problem. > > > > > https://www.redhat.com/archives/libvir-list/2018-March/msg01935.html > > > > This will allow us to have a single copy of the formatter and > > additionally it will not depend on the library. > > That means that we are basically reinventing JSON formatting & escaping > rules in our code. I don't think that would be a step forward. I wisha What I don't find being a step forwar is that when we are formatting the JSON with current approach we are basically translating our internal virJSONValue tree into a second copy of that tree represented by the janson data types, which is then used to do the formatting.

Since the escaping rules are simple enough in case of JSON and are fully tested I don't really see a problem with that. > we could someday get rid of our use of virBuffer for formatting XML too > and rely on a XML library for formatting just as we do for JSON. So are we going to ditch libxml2 eventually?