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

[Daniel P. Berrangé]

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 at redhat.com>
> > ---
> >  configure.ac       |   1 +
> >  m4/virt-json.m4    |  55 +++++++++++---
> >  src/util/virjson.c | 219 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 264 insertions(+), 11 deletions(-)
> 
> > diff --git a/m4/virt-json.m4 b/m4/virt-json.m4
> > index 5e4bcc7c9..a5ae3edcd 100644
> > --- a/m4/virt-json.m4
> > +++ b/m4/virt-json.m4
> 
> 
> > diff --git a/src/util/virjson.c b/src/util/virjson.c
> > index 6a02ddf0c..86cbd6eef 100644
> > --- a/src/util/virjson.c
> > +++ b/src/util/virjson.c
> > @@ -1951,6 +1951,225 @@ virJSONValueToString(virJSONValuePtr object,
> >  }
> >  
> >  
> > +#elif WITH_JANSSON
> > +# include <jansson.h>
> > +
> > +static virJSONValuePtr
> > +virJSONValueFromJansson(json_t *json)
> > +{
> > +    virJSONValuePtr ret = NULL;
> > +    const char *key;
> > +    json_t *cur;
> > +    size_t i;
> > +
> > +    switch (json_typeof(json)) {
> > +    case JSON_OBJECT:
> > +        ret = virJSONValueNewObject();
> > +        if (!ret)
> > +            goto error;
> > +
> > +        json_object_foreach(json, key, cur) {
> > +            virJSONValuePtr val = virJSONValueFromJansson(cur);
> > +            if (!val)
> > +                goto error;
> > +
> > +            if (virJSONValueObjectAppend(ret, key, val) < 0)
> > +                goto error;
> 
> 'val' will be leaked on failure
> 
> > +        }
> > +
> > +        break;
> > +
> > +    case JSON_ARRAY:
> > +        ret = virJSONValueNewArray();
> > +        if (!ret)
> > +            goto error;
> > +
> > +        json_array_foreach(json, i, cur) {
> > +            virJSONValuePtr val = virJSONValueFromJansson(cur);
> > +            if (!val)
> > +                goto error;
> > +
> > +            if (virJSONValueArrayAppend(ret, val) < 0)
> > +                goto error;
> 
> here too
> 
> > +        }
> > +        break;
> > +
> > +    case JSON_STRING:
> > +        ret = virJSONValueNewStringLen(json_string_value(json),
> > +                                       json_string_length(json));
> > +        break;
> > +
> > +    case JSON_INTEGER:
> > +        ret = virJSONValueNewNumberLong(json_integer_value(json));
> > +        break;
> > +
> > +    case JSON_REAL:
> > +        ret = virJSONValueNewNumberDouble(json_real_value(json));
> > +        break;
> 
> After mi privatization of struct _virJSONValue it should be simple
> enough to add the same differetiation to our code.
> 
> Not sure whether that's worth though.
> 
> > +
> > +    case JSON_TRUE:
> > +    case JSON_FALSE:
> > +        ret = virJSONValueNewBoolean(json_boolean_value(json));
> > +        break;
> > +
> > +    case JSON_NULL:
> > +        ret = virJSONValueNewNull();
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +
> > + error:
> > +    virJSONValueFree(ret);
> > +    return NULL;
> > +}
> > +
> > +virJSONValuePtr
> > +virJSONValueFromString(const char *jsonstring)
> > +{
> > +    virJSONValuePtr ret = NULL;
> > +    json_t *json;
> > +    json_error_t error;
> > +    size_t flags = JSON_REJECT_DUPLICATES |
> > +                   JSON_DECODE_ANY;
> > +
> > +    if (!(json = json_loads(jsonstring, flags, &error))) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("failed to parse JSON %d:%d: %s"),
> > +                       error.line, error.column, error.text);
> > +        return NULL;
> > +    }
> > +
> > +    ret = virJSONValueFromJansson(json);
> > +    json_decref(json);
> > +    return ret;
> > +}
> > +
> > +
> > +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 wish
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.

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 :|