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

3 Apr 2018


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

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

Daniel P. Berrangé