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

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