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

[Daniel P . Berrangé]

On Tue, Apr 03, 2018 at 12:45:28PM +0200, Peter Krempa wrote:
> 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 
> > > > ---
> > > >  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.

We could potentially change virJSONValue so that it is more of a wrapper
for the janson data type, so we don't have the translation step in terms
of data storage.

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

I doubt it, I just didn't want to imply that we must use libxml for
formatting. We could use libxml, but we could use an alternate if
there was one better suited to it.

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

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

[Peter Krempa]

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


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [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 
> > ---
> >  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 
> > +
> > +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, ))) {
> > +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 

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

[Peter Krempa]

On Sat, Mar 31, 2018 at 11:13:13 +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 
> > ---
> >  configure.ac   |   1 +
> >  m4/virt-json.m4|  55 +++---
> >  src/util/virjson.c | 219 
> > +
> >  3 files changed, 264 insertions(+), 11 deletions(-)

[...]

> > +
> > +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.

As a side-note, the qemu QAPI schema differentiates these as well, thus
this change would make our schema validator more strict. Also it would
possibly have some test fallout


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Peter Krempa]

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 
> ---
>  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 
> +
> +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, ))) {
> +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.

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.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list