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

On Fri, Sep 06, 2024 at 11:46:16 +0200, Ján Tomko wrote: > 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 Sigh. Okay I'll be more direct next time.

> > > + 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? I prefer json_object_new_null()

> And why would either of those require a comment? Returning NULL directly does require a comment. Espeically since it's right next to the 'default:' case which would signify a programming error which returns exactly the same value. It looked wrong and made me look it up. > Jano > > > > > > + default: > > > + return NULL; > > > + } > > > + return NULL; > > > +} > > > > Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; > >