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

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Friday in 2024, Peter Krempa wrote:
> 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@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
>
> Sigh. Okay I'll be more direct next time.

I've changed it to use the less punk-ish C version:

    case json_type_object: {
        json_object_iter iter;

        ret = virJSONValueNewObject();

        json_object_object_foreachC(jobj, iter) {
            virJSONValue *cur = virJSONValueFromJsonC(iter.val);

            if (virJSONValueObjectAppend(ret, iter.key, &cur) < 0) {
                g_free(ret);
                return NULL;
            }
        }
        break;
    }


[..]
> > > > +        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()

Done.

Jano

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

Attachment: signature.asc
Description: PGP signature