On a Friday in 2024, Peter Krempa wrote:
On Thu, Sep 05, 2024 at 15:49:36 +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@xxxxxxxxxx> --- src/util/virjson.c | 167 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 165 insertions(+), 2 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index 0edf86cd1c..8e0ba47fc9 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -32,7 +32,9 @@ #include "virenum.h" #include "virbitmap.h" -#if WITH_YAJL +#if WITH_JSON_C +# include <json.h> +#elif WITH_YAJL # include <yajl/yajl_gen.h> # include <yajl/yajl_parse.h> @@ -1390,7 +1392,168 @@ virJSONValueCopy(const virJSONValue *in) } -#if WITH_YAJL +#if WITH_JSON_C +static virJSONValue * +virJSONValueFromJsonC(json_object *jobj) +{ + enum json_type type = json_object_get_type(jobj); + virJSONValue *ret = NULL; + + 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(); + {Unaddressed from previous review. Either everything inside the block or no block at all.
Previous review:
This block-in-case style is very punk.
I think leaving it very-punkish addressed your comments just fine :P
+ 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: {
[...]
+ 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. */Unaddressed from previous review: /* Yes. That's a random value. json-c will use the provided string representation in the JSON document it outputs. The 'json_object' tree will not be used outside of this function and thus the actual numeric value is irrelevant. */
That doesn't sound right. The json_object will be used outside of this function, but the formatter will use the string instead of the numeric value.
+ return json_object_new_double_s(299792458, object->data.number); + } + case VIR_JSON_TYPE_BOOLEAN: + return json_object_new_boolean(object->data.boolean); + + case VIR_JSON_TYPE_NULL:return json_object_new_null(); Which should be available in 0.14 we require. But yes I'm aware that it'd be correct without: struct json_object *json_object_new_null(void) { return NULL; } In case you'd want to stick with 'return NULL' this will require a comment and I'll like to see it before comitting.
I am confused here, do you prefer json_object_new_null() or returning NULL directly? And why would either of those require a comment? Jano
+ default: + return NULL; + } + return NULL; +}Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
