8:50 a.m.
On Wed, Aug 14, 2024 at 23:40:22 +0200, Ján Tomko wrote:
Write an alternative implementation of our virJSON functions,
using json-c instead of yajl.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/util/virjson.c | 177 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 175 insertions(+), 2 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 0edf86cd1c..7a22c54f03 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
[...]
@@ -1389,8 +1391,179 @@ virJSONValueCopy(const virJSONValue *in)
return out;
}
Two lines.
+#if WITH_JSON_C
+static virJSONValue *
+virJSONValueFromJsonC(json_object *jobj)
Hmm, I wonder how much feasible is it to rewrite the helpers to avoid
this conversion in the future.
+{
+ enum json_type type = json_object_get_type(jobj);
+ virJSONValue *ret = NULL;
-#if WITH_YAJL
+ switch (type) {
+ case json_type_null:
+ ret = virJSONValueNewNull();
+ break;
+ case json_type_boolean:
+ ret = virJSONValueNewBoolean(json_object_get_boolean(jobj));
+ break;
+ case json_type_double:
+ case json_type_int: {
+ ret = virJSONValueNewNumber(g_strdup(json_object_get_string(jobj)));
+ break;
+ }
+ case json_type_object:
+ ret = virJSONValueNewObject();
+ {
This block-in-case style is very punk.
+ json_object_object_foreach(jobj, key, val) {
+ virJSONValue *cur = virJSONValueFromJsonC(val);
+
+ if (virJSONValueObjectAppend(ret, key, &cur) < 0) {
+ g_free(ret);
+ return NULL;
+ }
+ }
+ }
+ break;
+ case json_type_array: {
+ size_t len;
+ size_t i;
+
+ ret = virJSONValueNewArray();
+ len = json_object_array_length(jobj);
+
+ for (i = 0; i < len; i++) {
+ virJSONValue *cur = NULL;
+ json_object *val = NULL;
+
+ val = json_object_array_get_idx(jobj, i);
+
+ cur = virJSONValueFromJsonC(val);
+
+ virJSONValueArrayAppend(ret, &cur);
+ }
+ break;
+ }
+ case json_type_string:
+ ret = virJSONValueNewString(g_strdup(json_object_get_string(jobj)));
+ break;
+ }
+
+ VIR_DEBUG("ret=%p", ret);
I doubt this is useful. Either drop it or print at least the 'type'
field.
+ return ret;
+}
+
+virJSONValue *
Two lines.
+virJSONValueFromString(const char *jsonstring)
+{
+ json_object *jobj = NULL;
+ virJSONValue *ret = NULL;
+ json_tokener *tok = NULL;
+ enum json_tokener_error jerr;
+ int jsonflags = JSON_TOKENER_STRICT | JSON_TOKENER_VALIDATE_UTF8;
+
+ VIR_DEBUG("string=%s", jsonstring);
+
+ tok = json_tokener_new();
+ json_tokener_set_flags(tok, jsonflags);
+ jobj = json_tokener_parse_ex(tok, jsonstring, strlen(jsonstring));
+ jerr = json_tokener_get_error(tok);
+ if (jerr != json_tokener_success) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", json_tokener_error_desc(jerr));
+ goto cleanup;
+ }
+ ret = virJSONValueFromJsonC(jobj);
+
+ cleanup:
+ VIR_DEBUG("result=%p", ret);
So this effectively will always be a pointer except when the
json_tokener_parse_ex returned error, but in that case we'd already
report an error. Is it really needed?
+ json_object_put(jobj);
+ json_tokener_free(tok);
+ return ret;
+}
+
+static json_object *
+virJSONValueToJsonC(virJSONValue *object)
+{
+ json_object *ret = NULL;
+ size_t i;
+
+ VIR_DEBUG("object=%p type=%d", object, object->type);
^^ this is imo more useful.
+
+ switch (object->type) {
+ case VIR_JSON_TYPE_OBJECT:
+ // constant key?
c99 comment.
+ ret = json_object_new_object();
+ for (i = 0; i < object->data.object.npairs; i++) {
+ json_object *child =
virJSONValueToJsonC(object->data.object.pairs[i].value);
+ json_object_object_add(ret, object->data.object.pairs[i].key, child);
+ }
+ return ret;
+ case VIR_JSON_TYPE_ARRAY:
+ /* json_object_new_array_ext was introduced in json-c 0.16 */
+# if JSON_C_VERSION_NUM < ((0 << 16) | (16 << 8))
+ ret = json_object_new_array();
+# else
+ ret = json_object_new_array_ext(object->data.array.nvalues);
+# endif
+ for (i = 0; i < object->data.array.nvalues; i++) {
+ json_object_array_add(ret,
+
virJSONValueToJsonC(object->data.array.values[i]));
+ }
+ return ret;
+
+ case VIR_JSON_TYPE_STRING:
+ return json_object_new_string(object->data.string);
+
+ case VIR_JSON_TYPE_NUMBER: {
+ /* Yes. That's a random value. We only care about the string. */
+ return json_object_new_double_s(299792458, object->data.number);
oof. Shouldn't we at least attempt to format the number into a double?
I understand that you're using this to allow for the exact
representation and that a cannary value might ease debugging but this
looks a bit weird. At least add a comment explaining that it's a cannary
and a hack so that json-c will use our representation.
+ }
+ case VIR_JSON_TYPE_BOOLEAN:
+ return json_object_new_boolean(object->data.boolean);
+
+ case VIR_JSON_TYPE_NULL:
+ default:
+ return NULL;
+ }
+ return NULL;
+}
+
+
+int
+virJSONValueToBuffer(virJSONValue *object,
+ virBuffer *buf,
+ bool pretty)
+{
+ json_object *jobj = NULL;
+ const char *str;
+ size_t len;
+ int ret = -1;
+ int jsonflags = JSON_C_TO_STRING_NOSLASHESCAPE;
+
+ VIR_DEBUG("object=%p", object);
+
+ if (pretty)
+ jsonflags |= JSON_C_TO_STRING_PRETTY | JSON_C_TO_STRING_SPACED;
+
+ jobj = virJSONValueToJsonC(object);
+ if (!jobj) {
So here you check the return value of 'virJSONValueToJsonC' but none of
the other calls in this function do that.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to convert virJSONValue to json-c data"));
+ goto cleanup;
+ }
+
+ str = json_object_to_json_string_length(jobj, jsonflags, &len);
+
+ virBufferAdd(buf, str, len);
+ if (pretty)
+ virBufferAddLit(buf, "\n");
+ ret = 0;
+
+ cleanup:
+ json_object_put(jobj);
+ return ret;
+}
+
Two lines.
+#elif WITH_YAJL
Looks good, but I want to understand the error reporting discrepancy
I've pointed out above before giving r-b.