Re: [libvirt PATCHv2 09/15] util: json: write a json-c implementation

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(a)redhat.com&gt; > --- > 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.

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

> + 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. */

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

> + default: > + return NULL; > + } > + return NULL; > +} Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;