[libvirt] [PATCH] util: json: Remove yajl bits from virJSONValueToStr

Peter Krempa posted 1 patch 5 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e96dadddc81397712dbe920c85d1267a7d195825.1522486847.git.pkrempa@redhat.com
Test syntax-check failed
src/util/virjson.c | 188 ++++++++++++++++++++++++++++-------------------------
1 file changed, 101 insertions(+), 87 deletions(-)
[libvirt] [PATCH] util: json: Remove yajl bits from virJSONValueToStr
Posted by Peter Krempa 5 years, 12 months ago
Rather than depending on yajl bits for creating the JSON structure
replace it by few virBuffer bits. This will make the JSON formatter
libary agnostic.

Additionally remove the debug statement from the worker function since
it was not very useful.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/util/virjson.c | 188 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 101 insertions(+), 87 deletions(-)

diff --git a/src/util/virjson.c b/src/util/virjson.c
index 6a02ddf0cc..772a205e9e 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1834,144 +1834,158 @@ virJSONValueFromString(const char *jsonstring)
     return ret;
 }

+#else
+virJSONValuePtr
+virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
+{
+    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                   _("No JSON parser implementation is available"));
+    return NULL;
+}
+#endif
+
+
+static void
+virJSONValueToStringAddString(virBufferPtr buf,
+                              virJSONValuePtr string)
+{
+    const char *t;
+
+    virBufferAddLit(buf, "\"");
+
+    for (t = string->data.string; *t; t++) {
+        switch (*t) {
+        case '"':
+            virBufferAddLit(buf, "\\\"");
+            break;
+        case '\\':
+            virBufferAddLit(buf, "\\\\");
+            break;
+        case '\n':
+            virBufferAddLit(buf, "\\n");
+            break;
+        case '\t':
+            virBufferAddLit(buf, "\\t");
+            break;
+        default:
+            virBufferAdd(buf, t, 1);
+            break;
+        }
+    }
+
+    virBufferAddLit(buf, "\"");
+}
+
+
+#define VIR_JSON_PRETTY_NEWLINE \
+    if (pretty) \
+        virBufferAddLit(buf, "\n")

 static int
 virJSONValueToStringOne(virJSONValuePtr object,
-                        yajl_gen g)
+                        virBufferPtr buf,
+                        bool pretty)
 {
     size_t i;

-    VIR_DEBUG("object=%p type=%d gen=%p", object, object->type, g);
-
-    switch (object->type) {
+    switch ((virJSONType) object->type) {
     case VIR_JSON_TYPE_OBJECT:
-        if (yajl_gen_map_open(g) != yajl_gen_status_ok)
-            return -1;
+        virBufferAddLit(buf, "{");
+        VIR_JSON_PRETTY_NEWLINE;
+        virBufferAdjustIndent(buf, 2);
+
         for (i = 0; i < object->data.object.npairs; i++) {
-            if (yajl_gen_string(g,
-                                (unsigned char *)object->data.object.pairs[i].key,
-                                strlen(object->data.object.pairs[i].key))
-                                != yajl_gen_status_ok)
-                return -1;
-            if (virJSONValueToStringOne(object->data.object.pairs[i].value, g) < 0)
+            virBufferStrcat(buf, "\"", object->data.object.pairs[i].key, "\":", NULL);
+
+            if (pretty)
+                virBufferAddLit(buf, " ");
+
+            if (virJSONValueToStringOne(object->data.object.pairs[i].value,
+                                        buf, pretty) < 0)
                 return -1;
+
+            if (i != object->data.object.npairs - 1) {
+                virBufferAddLit(buf, ",");
+                VIR_JSON_PRETTY_NEWLINE;
+            }
         }
-        if (yajl_gen_map_close(g) != yajl_gen_status_ok)
-            return -1;
+
+        virBufferAdjustIndent(buf, -2);
+        VIR_JSON_PRETTY_NEWLINE;
+        virBufferAddLit(buf, "}");
         break;
+
     case VIR_JSON_TYPE_ARRAY:
-        if (yajl_gen_array_open(g) != yajl_gen_status_ok)
-            return -1;
+        virBufferAddLit(buf, "[");
+        VIR_JSON_PRETTY_NEWLINE;
+        virBufferAdjustIndent(buf, 2);
+
         for (i = 0; i < object->data.array.nvalues; i++) {
-            if (virJSONValueToStringOne(object->data.array.values[i], g) < 0)
+            if (virJSONValueToStringOne(object->data.array.values[i], buf, pretty) < 0)
                 return -1;
+
+            if (i != object->data.array.nvalues - 1) {
+                virBufferAddLit(buf, ",");
+                VIR_JSON_PRETTY_NEWLINE;
+            }
         }
-        if (yajl_gen_array_close(g) != yajl_gen_status_ok)
-            return -1;
+
+        virBufferAdjustIndent(buf, -2);
+        VIR_JSON_PRETTY_NEWLINE;
+        virBufferAddLit(buf, "]");
         break;

     case VIR_JSON_TYPE_STRING:
-        if (yajl_gen_string(g, (unsigned char *)object->data.string,
-                            strlen(object->data.string)) != yajl_gen_status_ok)
-            return -1;
+        virJSONValueToStringAddString(buf, object);
         break;

     case VIR_JSON_TYPE_NUMBER:
-        if (yajl_gen_number(g, object->data.number,
-                            strlen(object->data.number)) != yajl_gen_status_ok)
-            return -1;
+        virBufferAdd(buf, object->data.number, -1);
         break;

     case VIR_JSON_TYPE_BOOLEAN:
-        if (yajl_gen_bool(g, object->data.boolean) != yajl_gen_status_ok)
-            return -1;
+        if (object->data.boolean)
+            virBufferAddLit(buf, "true");
+        else
+            virBufferAddLit(buf, "false");
         break;

     case VIR_JSON_TYPE_NULL:
-        if (yajl_gen_null(g) != yajl_gen_status_ok)
-            return -1;
+        virBufferAddLit(buf, "null");
         break;

     default:
+        virReportEnumRangeError(virJSONType, object->type);
         return -1;
     }

     return 0;
 }

+#undef VIR_JSON_PRETTY_NEWLINE
+

 char *
 virJSONValueToString(virJSONValuePtr object,
                      bool pretty)
 {
-    yajl_gen g;
-    const unsigned char *str;
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
     char *ret = NULL;
-    yajl_size_t len;
-# ifndef WITH_YAJL2
-    yajl_gen_config conf = { pretty ? 1 : 0, pretty ? "  " : " "};
-# endif

     VIR_DEBUG("object=%p", object);

-# ifdef WITH_YAJL2
-    g = yajl_gen_alloc(NULL);
-    if (g) {
-        yajl_gen_config(g, yajl_gen_beautify, pretty ? 1 : 0);
-        yajl_gen_config(g, yajl_gen_indent_string, pretty ? "  " : " ");
-        yajl_gen_config(g, yajl_gen_validate_utf8, 1);
-    }
-# else
-    g = yajl_gen_alloc(&conf, NULL);
-# endif
-    if (!g) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Unable to create JSON formatter"));
-        goto cleanup;
-    }
-
-    if (virJSONValueToStringOne(object, g) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
-    if (yajl_gen_get_buf(g, &str, &len) != yajl_gen_status_ok) {
-        virReportOOMError();
-        goto cleanup;
+    if (virJSONValueToStringOne(object, &buf, pretty) < 0 ||
+        virBufferCheckError(&buf) < 0) {
+        virBufferFreeAndReset(&buf);
+        return NULL;
     }

-    ignore_value(VIR_STRDUP(ret, (const char *)str));
-
- cleanup:
-    yajl_gen_free(g);
-
-    VIR_DEBUG("result=%s", NULLSTR(ret));
-
+    ret = virBufferContentAndReset(&buf);
+    VIR_DEBUG("result=%s", ret);
     return ret;
 }


-#else
-virJSONValuePtr
-virJSONValueFromString(const char *jsonstring ATTRIBUTE_UNUSED)
-{
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("No JSON parser implementation is available"));
-    return NULL;
-}
-
-
-char *
-virJSONValueToString(virJSONValuePtr object ATTRIBUTE_UNUSED,
-                     bool pretty ATTRIBUTE_UNUSED)
-{
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("No JSON parser implementation is available"));
-    return NULL;
-}
-#endif
-
-
 /**
  * virJSONStringReformat:
  * @jsonstr: string to reformat
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] util: json: Remove yajl bits from virJSONValueToStr
Posted by Daniel P. Berrangé 5 years, 11 months ago
On Sat, Mar 31, 2018 at 11:01:15AM +0200, Peter Krempa wrote:
> Rather than depending on yajl bits for creating the JSON structure
> replace it by few virBuffer bits. This will make the JSON formatter
> libary agnostic.

I don't think this is a good idea as it means we have to reinvent the
wheel to ensure that we are correctly formatting & escaping JSON. This
patch gets escaping wrong which illustrates the point :-(

It has also discarded the utf8 validation that the old code did.


> +static void
> +virJSONValueToStringAddString(virBufferPtr buf,
> +                              virJSONValuePtr string)
> +{
> +    const char *t;
> +
> +    virBufferAddLit(buf, "\"");
> +
> +    for (t = string->data.string; *t; t++) {
> +        switch (*t) {
> +        case '"':
> +            virBufferAddLit(buf, "\\\"");
> +            break;
> +        case '\\':
> +            virBufferAddLit(buf, "\\\\");
> +            break;
> +        case '\n':
> +            virBufferAddLit(buf, "\\n");
> +            break;
> +        case '\t':
> +            virBufferAddLit(buf, "\\t");
> +            break;

Missing \r, \f, \b. Also missing hex escaping of
non printable characters.

> +        default:
> +            virBufferAdd(buf, t, 1);
> +            break;
> +        }
> +    }
> +
> +    virBufferAddLit(buf, "\"");
> +}
> +
> +
> +#define VIR_JSON_PRETTY_NEWLINE \
> +    if (pretty) \
> +        virBufferAddLit(buf, "\n")
> 
>  static int
>  virJSONValueToStringOne(virJSONValuePtr object,
> -                        yajl_gen g)
> +                        virBufferPtr buf,
> +                        bool pretty)
>  {
>      size_t i;
> 
> -    VIR_DEBUG("object=%p type=%d gen=%p", object, object->type, g);
> -
> -    switch (object->type) {
> +    switch ((virJSONType) object->type) {
>      case VIR_JSON_TYPE_OBJECT:
> -        if (yajl_gen_map_open(g) != yajl_gen_status_ok)
> -            return -1;
> +        virBufferAddLit(buf, "{");
> +        VIR_JSON_PRETTY_NEWLINE;
> +        virBufferAdjustIndent(buf, 2);
> +
>          for (i = 0; i < object->data.object.npairs; i++) {
> -            if (yajl_gen_string(g,
> -                                (unsigned char *)object->data.object.pairs[i].key,
> -                                strlen(object->data.object.pairs[i].key))
> -                                != yajl_gen_status_ok)
> -                return -1;
> -            if (virJSONValueToStringOne(object->data.object.pairs[i].value, g) < 0)
> +            virBufferStrcat(buf, "\"", object->data.object.pairs[i].key, "\":", NULL);

Missing escaping of key.

> +
> +            if (pretty)
> +                virBufferAddLit(buf, " ");
> +
> +            if (virJSONValueToStringOne(object->data.object.pairs[i].value,
> +                                        buf, pretty) < 0)
>                  return -1;
> +
> +            if (i != object->data.object.npairs - 1) {
> +                virBufferAddLit(buf, ",");
> +                VIR_JSON_PRETTY_NEWLINE;
> +            }
>          }
> -        if (yajl_gen_map_close(g) != yajl_gen_status_ok)
> -            return -1;
> +
> +        virBufferAdjustIndent(buf, -2);
> +        VIR_JSON_PRETTY_NEWLINE;
> +        virBufferAddLit(buf, "}");
>          break;
> +
>      case VIR_JSON_TYPE_ARRAY:
> -        if (yajl_gen_array_open(g) != yajl_gen_status_ok)
> -            return -1;
> +        virBufferAddLit(buf, "[");
> +        VIR_JSON_PRETTY_NEWLINE;
> +        virBufferAdjustIndent(buf, 2);
> +
>          for (i = 0; i < object->data.array.nvalues; i++) {
> -            if (virJSONValueToStringOne(object->data.array.values[i], g) < 0)
> +            if (virJSONValueToStringOne(object->data.array.values[i], buf, pretty) < 0)
>                  return -1;
> +
> +            if (i != object->data.array.nvalues - 1) {
> +                virBufferAddLit(buf, ",");
> +                VIR_JSON_PRETTY_NEWLINE;
> +            }
>          }
> -        if (yajl_gen_array_close(g) != yajl_gen_status_ok)
> -            return -1;
> +
> +        virBufferAdjustIndent(buf, -2);
> +        VIR_JSON_PRETTY_NEWLINE;
> +        virBufferAddLit(buf, "]");
>          break;
> 
>      case VIR_JSON_TYPE_STRING:
> -        if (yajl_gen_string(g, (unsigned char *)object->data.string,
> -                            strlen(object->data.string)) != yajl_gen_status_ok)
> -            return -1;
> +        virJSONValueToStringAddString(buf, object);
>          break;
> 
>      case VIR_JSON_TYPE_NUMBER:
> -        if (yajl_gen_number(g, object->data.number,
> -                            strlen(object->data.number)) != yajl_gen_status_ok)
> -            return -1;
> +        virBufferAdd(buf, object->data.number, -1);
>          break;
> 
>      case VIR_JSON_TYPE_BOOLEAN:
> -        if (yajl_gen_bool(g, object->data.boolean) != yajl_gen_status_ok)
> -            return -1;
> +        if (object->data.boolean)
> +            virBufferAddLit(buf, "true");
> +        else
> +            virBufferAddLit(buf, "false");
>          break;
> 
>      case VIR_JSON_TYPE_NULL:
> -        if (yajl_gen_null(g) != yajl_gen_status_ok)
> -            return -1;
> +        virBufferAddLit(buf, "null");
>          break;
> 
>      default:
> +        virReportEnumRangeError(virJSONType, object->type);
>          return -1;
>      }
> 
>      return 0;
>  }

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] util: json: Remove yajl bits from virJSONValueToStr
Posted by Peter Krempa 5 years, 11 months ago
On Tue, Apr 03, 2018 at 12:23:13 +0100, Daniel Berrange wrote:
> On Sat, Mar 31, 2018 at 11:01:15AM +0200, Peter Krempa wrote:
> > Rather than depending on yajl bits for creating the JSON structure
> > replace it by few virBuffer bits. This will make the JSON formatter
> > libary agnostic.
> 
> I don't think this is a good idea as it means we have to reinvent the
> wheel to ensure that we are correctly formatting & escaping JSON. This
> patch gets escaping wrong which illustrates the point :-(
> 
> It has also discarded the utf8 validation that the old code did.

Fair enough. I honestly only cared about the values which we
encountered.

What triggered me to send this patch was, that the proposal to add
another library added yet another place where virJSONValues were
converted to some internal type back and forth, which seems utterly
inefficient.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list