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 <[email protected]>
> > > ---
> > > 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.
> > > + 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.
You are technically correct. 'json_object' will be used outside, but
strictly to format the json. So:
/* Yes. That's a random value. json-c will use the provided
string representation in the JSON document it outputs. The
'json_object' tree created here is not used for anything
else besides the JSON string output, thus the actual numeric
value doesn't matter. */
> > > + 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 <[email protected]>
> >
