Yajl has not seen much activity upstream recently.
Switch to using Jansson >= 2.7.
All the platforms we target on https://libvirt.org/platforms.html
have a version >= 2.7 listed on the sites below:
https://repology.org/metapackage/jansson/versions
https://build.opensuse.org/package/show/devel:libraries:c_c++/libjansson
Implement virJSONValue{From,To}String using Jansson, delete the yajl
code (and the related virJSONParser structure) and report an error
if someone explicitly specifies --with-yajl.
Also adjust the test data to account for Jansson's different whitespace
usage for empty arrays and tune up the specfile to keep 'make rpm'
working when bisecting.
Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 211 insertions(+)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 0559d40b64..2f7d624bb3 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -2008,6 +2008,217 @@ 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) {
+ virJSONValueFree(val);
+ goto error;
+ }
+ }
+
+ 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) {
+ virJSONValueFree(val);
+ goto error;
+ }
+ }
+ 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;
+
+ 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_t *ret = NULL;
+ size_t i;
+
+ switch ((virJSONType)object->type) {
+ case VIR_JSON_TYPE_OBJECT:
+ ret = json_object();
+ if (!ret) {
+ virReportOOMError();
+ goto error;
+ }
+ for (i = 0; i < object->data.object.npairs; i++) {
+ virJSONObjectPairPtr cur = object->data.object.pairs + i;
+ json_t *val = virJSONValueToJansson(cur->value);
+
+ if (!val)
+ goto error;
+ if (json_object_set_new(ret, cur->key, val) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ }
+ break;
+
+ case VIR_JSON_TYPE_ARRAY:
+ ret = json_array();
+ if (!ret) {
+ virReportOOMError();
+ goto error;
+ }
+ for (i = 0; i < object->data.array.nvalues; i++) {
+ virJSONValuePtr cur = object->data.array.values[i];
+ json_t *val = virJSONValueToJansson(cur);
+
+ if (!val)
+ goto error;
+ if (json_array_append_new(ret, val) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ }
+ break;
+
+ case VIR_JSON_TYPE_STRING:
+ ret = json_string(object->data.string);
+ break;
+
+ case VIR_JSON_TYPE_NUMBER: {
+ long long ll_val;
+ double d_val;
+ if (virStrToLong_ll(object->data.number, NULL, 10, &ll_val) < 0) {
+ if (virStrToDouble(object->data.number, NULL, &d_val) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("JSON value is not a number"));
+ return NULL;
+ }
+ ret = json_real(d_val);
+ } else {
+ ret = json_integer(ll_val);
+ }
+ }
+ break;
+
+ case VIR_JSON_TYPE_BOOLEAN:
+ ret = json_boolean(object->data.boolean);
+ break;
+
+ case VIR_JSON_TYPE_NULL:
+ ret = json_null();
+ break;
+
+ default:
+ virReportEnumRangeError(virJSONType, object->type);
+ goto error;
+ }
+ if (!ret)
+ virReportOOMError();
+ return ret;
+
+ error:
+ json_decref(ret);
+ return NULL;
+}
+
+
+char *
+virJSONValueToString(virJSONValuePtr object,
+ bool pretty)
+{
+ size_t flags = JSON_ENCODE_ANY;
+ json_t *json;
+ char *str = NULL;
+
+ if (pretty)
+ flags |= JSON_INDENT(2);
+ else
+ flags |= JSON_COMPACT;
+
+ json = virJSONValueToJansson(object);
+ if (!json)
+ return NULL;
+
+ str = json_dumps(json, flags);
+ if (!str)
+ virReportOOMError();
+ json_decref(json);
+ return str;
+}
+
+
#else
virJSONValuePtr
virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
--
2.16.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn Fri, May 11, 2018 at 02:59:04PM +0200, Ján Tomko wrote:
>Yajl has not seen much activity upstream recently.
>Switch to using Jansson >= 2.7.
>
>All the platforms we target on https://libvirt.org/platforms.html
>have a version >= 2.7 listed on the sites below:
>https://repology.org/metapackage/jansson/versions
>https://build.opensuse.org/package/show/devel:libraries:c_c++/libjansson
>
>Implement virJSONValue{From,To}String using Jansson, delete the yajl
>code (and the related virJSONParser structure) and report an error
>if someone explicitly specifies --with-yajl.
>
>Also adjust the test data to account for Jansson's different whitespace
>usage for empty arrays and tune up the specfile to keep 'make rpm'
>working when bisecting.
>
>Signed-off-by: Ján Tomko <jtomko@redhat.com>
>---
> src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 211 insertions(+)
>
With this change, it should be possible to build with Jansson 2.5 as
well:
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 2f7d624bb3..0d7d368c8a 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -2056,8 +2056,7 @@ virJSONValueFromJansson(json_t *json)
break;
case JSON_STRING:
- ret = virJSONValueNewStringLen(json_string_value(json),
- json_string_length(json));
+ ret = virJSONValueNewString(json_string_value(json));
break;
case JSON_INTEGER:
@@ -2070,7 +2069,7 @@ virJSONValueFromJansson(json_t *json)
case JSON_TRUE:
case JSON_FALSE:
- ret = virJSONValueNewBoolean(json_boolean_value(json));
+ ret = virJSONValueNewBoolean(json_is_true(json));
break;
case JSON_NULL:
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 15, 2018 at 14:45:05 +0200, Ján Tomko wrote:
> On Fri, May 11, 2018 at 02:59:04PM +0200, Ján Tomko wrote:
> > Yajl has not seen much activity upstream recently.
> > Switch to using Jansson >= 2.7.
> >
> > All the platforms we target on https://libvirt.org/platforms.html
> > have a version >= 2.7 listed on the sites below:
> > https://repology.org/metapackage/jansson/versions
> > https://build.opensuse.org/package/show/devel:libraries:c_c++/libjansson
> >
> > Implement virJSONValue{From,To}String using Jansson, delete the yajl
> > code (and the related virJSONParser structure) and report an error
> > if someone explicitly specifies --with-yajl.
> >
> > Also adjust the test data to account for Jansson's different whitespace
> > usage for empty arrays and tune up the specfile to keep 'make rpm'
> > working when bisecting.
> >
> > Signed-off-by: Ján Tomko <jtomko@redhat.com>
> > ---
> > src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 211 insertions(+)
> >
>
> With this change, it should be possible to build with Jansson 2.5 as
> well:
> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 2f7d624bb3..0d7d368c8a 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -2056,8 +2056,7 @@ virJSONValueFromJansson(json_t *json)
> break;
>
> case JSON_STRING:
> - ret = virJSONValueNewStringLen(json_string_value(json),
> - json_string_length(json));
> + ret = virJSONValueNewString(json_string_value(json));
> break;
>
> case JSON_INTEGER:
> @@ -2070,7 +2069,7 @@ virJSONValueFromJansson(json_t *json)
>
> case JSON_TRUE:
> case JSON_FALSE:
> - ret = virJSONValueNewBoolean(json_boolean_value(json));
> + ret = virJSONValueNewBoolean(json_is_true(json));
Given how that macro works. I'd rather see following:
case JSON_TRUE:
ret = virJSONValueNewBoolean(true);
break;
case JSON_FALSE:
ret = virJSONValueNewBoolean(false);
break;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, May 11, 2018 at 14:59:04 +0200, Ján Tomko wrote:
> Yajl has not seen much activity upstream recently.
[0]
> Switch to using Jansson >= 2.7.
>
> All the platforms we target on https://libvirt.org/platforms.html
> have a version >= 2.7 listed on the sites below:
> https://repology.org/metapackage/jansson/versions
> https://build.opensuse.org/package/show/devel:libraries:c_c++/libjansson
>
> Implement virJSONValue{From,To}String using Jansson, delete the yajl
> code (and the related virJSONParser structure) and report an error
> if someone explicitly specifies --with-yajl.
>
> Also adjust the test data to account for Jansson's different whitespace
> usage for empty arrays and tune up the specfile to keep 'make rpm'
> working when bisecting.
>
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> ---
> src/util/virjson.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 211 insertions(+)
>
> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 0559d40b64..2f7d624bb3 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -2008,6 +2008,217 @@ virJSONValueToString(virJSONValuePtr object,
[...]
> +static json_t *
> +virJSONValueToJansson(virJSONValuePtr object)
> +{
> + json_t *ret = NULL;
> + size_t i;
> +
> + switch ((virJSONType)object->type) {
> + case VIR_JSON_TYPE_OBJECT:
> + ret = json_object();
> + if (!ret) {
> + virReportOOMError();
> + goto error;
> + }
> + for (i = 0; i < object->data.object.npairs; i++) {
> + virJSONObjectPairPtr cur = object->data.object.pairs + i;
> + json_t *val = virJSONValueToJansson(cur->value);
> +
> + if (!val)
> + goto error;
> + if (json_object_set_new(ret, cur->key, val) < 0) {
> + virReportOOMError();
'val' is leaked here.
> + goto error;
> + }
> + }
> + break;
> +
> + case VIR_JSON_TYPE_ARRAY:
> + ret = json_array();
> + if (!ret) {
> + virReportOOMError();
> + goto error;
> + }
> + for (i = 0; i < object->data.array.nvalues; i++) {
> + virJSONValuePtr cur = object->data.array.values[i];
> + json_t *val = virJSONValueToJansson(cur);
> +
> + if (!val)
> + goto error;
> + if (json_array_append_new(ret, val) < 0) {
> + virReportOOMError();
'val' is leaked here.
> + goto error;
> + }
> + }
> + break;
> +
> + case VIR_JSON_TYPE_STRING:
> + ret = json_string(object->data.string);
> + break;
> +
> + case VIR_JSON_TYPE_NUMBER: {
> + long long ll_val;
> + double d_val;
> + if (virStrToLong_ll(object->data.number, NULL, 10, &ll_val) < 0) {
> + if (virStrToDouble(object->data.number, NULL, &d_val) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("JSON value is not a number"));
> + return NULL;
> + }
> + ret = json_real(d_val);
> + } else {
> + ret = json_integer(ll_val);
> + }
> + }
> + break;
> +
> + case VIR_JSON_TYPE_BOOLEAN:
> + ret = json_boolean(object->data.boolean);
> + break;
> +
> + case VIR_JSON_TYPE_NULL:
> + ret = json_null();
> + break;
> +
> + default:
> + virReportEnumRangeError(virJSONType, object->type);
> + goto error;
> + }
> + if (!ret)
> + virReportOOMError();
> + return ret;
Few lines could be saved by having a dedicated label for cases when
virReportOOMError needs to be called.
> +
> + error:
> + json_decref(ret);
> + return NULL;
ACK if you fix the leaks. The separate label is up to you. Don't forget
to update the commit message after the squash-in.
Peter
[0] For anyone following current meme trends, this looks like it's
relevant to our YAJL->janson switch:
https://i.redditmedia.com/J46fZN24lFx3fMRlJNpkNEOFqU79zWTsRDBMla1u0HE.jpg?s=4757d31d1cbd758962407917e0d3aacb
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 15, 2018 at 03:05:55PM +0200, Peter Krempa wrote: > On Fri, May 11, 2018 at 14:59:04 +0200, Ján Tomko wrote: > > Yajl has not seen much activity upstream recently. > > [0] [snip] > [0] For anyone following current meme trends, this looks like it's > relevant to our YAJL->janson switch: > > https://i.redditmedia.com/J46fZN24lFx3fMRlJNpkNEOFqU79zWTsRDBMla1u0HE.jpg?s=4757d31d1cbd758962407917e0d3aacb If you think we should stay with YAJL because it "just works" then take a look at the bug reports against it upstream https://github.com/lloyd/yajl/issues double frees, memory leaks, parser gets stuck after parsing bad JSON, etc all ignored for years. The robustness of the JSON parser we use is critical to the security of the libvirt QEMU driver. I'm not saying janson is perfect, but it is at least maintained, so when problem in it as discovered, there's a real possibility to get them fixed, instead of ignored with the YAJL abandonware. 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
On Tue, May 15, 2018 at 14:21:07 +0100, Daniel Berrange wrote: > On Tue, May 15, 2018 at 03:05:55PM +0200, Peter Krempa wrote: > > On Fri, May 11, 2018 at 14:59:04 +0200, Ján Tomko wrote: > > > Yajl has not seen much activity upstream recently. > > > > [0] > > [snip] > > [0] For anyone following current meme trends, this looks like it's > > relevant to our YAJL->janson switch: > > > > https://i.redditmedia.com/J46fZN24lFx3fMRlJNpkNEOFqU79zWTsRDBMla1u0HE.jpg?s=4757d31d1cbd758962407917e0d3aacb > > If you think we should stay with YAJL because it "just works" then > take a look at the bug reports against it upstream > > https://github.com/lloyd/yajl/issues > > double frees, memory leaks, parser gets stuck after parsing bad JSON, etc > all ignored for years. The robustness of the JSON parser we use is > critical to the security of the libvirt QEMU driver. I'm not saying > janson is perfect, but it is at least maintained, so when problem in > it as discovered, there's a real possibility to get them fixed, instead > of ignored with the YAJL abandonware. No. I agree with the change, but "having serious flaws" is slightly different than "not very active upstream". -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 15, 2018 at 03:23:17PM +0200, Peter Krempa wrote: > On Tue, May 15, 2018 at 14:21:07 +0100, Daniel Berrange wrote: > > On Tue, May 15, 2018 at 03:05:55PM +0200, Peter Krempa wrote: > > > On Fri, May 11, 2018 at 14:59:04 +0200, Ján Tomko wrote: > > > > Yajl has not seen much activity upstream recently. > > > > > > [0] > > > > [snip] > > > [0] For anyone following current meme trends, this looks like it's > > > relevant to our YAJL->janson switch: > > > > > > https://i.redditmedia.com/J46fZN24lFx3fMRlJNpkNEOFqU79zWTsRDBMla1u0HE.jpg?s=4757d31d1cbd758962407917e0d3aacb > > > > If you think we should stay with YAJL because it "just works" then > > take a look at the bug reports against it upstream > > > > https://github.com/lloyd/yajl/issues > > > > double frees, memory leaks, parser gets stuck after parsing bad JSON, etc > > all ignored for years. The robustness of the JSON parser we use is > > critical to the security of the libvirt QEMU driver. I'm not saying > > janson is perfect, but it is at least maintained, so when problem in > > it as discovered, there's a real possibility to get them fixed, instead > > of ignored with the YAJL abandonware. > > No. I agree with the change, but "having serious flaws" is slightly > different than "not very active upstream". I don't think that makes much difference. All non-trivial software has serious flaws, especially if written in memory-unsafe languages like C. The only question is whether the flaws have been found yet. Given that, whether there is an active upstream maintaining it, is a key factor to decide whether to continue using something. The fact that we already see significant unfixed flaws just reinforces how important it is to have an active upstream. 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
© 2016 - 2025 Red Hat, Inc.