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