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@redhat.com> --- 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@redhat.com>