On Wed, Aug 14, 2024 at 23:40:22 +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 | 177 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 175 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index 0edf86cd1c..7a22c54f03 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
[...]
> @@ -1389,8 +1391,179 @@ virJSONValueCopy(const virJSONValue *in)
> return out;
> }
>
Two lines.
> +#if WITH_JSON_C
> +static virJSONValue *
> +virJSONValueFromJsonC(json_object *jobj)
Hmm, I wonder how much feasible is it to rewrite the helpers to avoid
this conversion in the future.
> +{
> + enum json_type type = json_object_get_type(jobj);
> + virJSONValue *ret = NULL;
>
> -#if WITH_YAJL
> + 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();
> + {
This block-in-case style is very punk.
> + 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: {
> + size_t len;
> + size_t i;
> +
> + ret = virJSONValueNewArray();
> + len = json_object_array_length(jobj);
> +
> + for (i = 0; i < len; i++) {
> + virJSONValue *cur = NULL;
> + json_object *val = NULL;
> +
> + val = json_object_array_get_idx(jobj, i);
> +
> + cur = virJSONValueFromJsonC(val);
> +
> + virJSONValueArrayAppend(ret, &cur);
> + }
> + break;
> + }
> + case json_type_string:
> + ret = virJSONValueNewString(g_strdup(json_object_get_string(jobj)));
> + break;
> + }
> +
> + VIR_DEBUG("ret=%p", ret);
I doubt this is useful. Either drop it or print at least the 'type'
field.
> + return ret;
> +}
> +
> +virJSONValue *
Two lines.
> +virJSONValueFromString(const char *jsonstring)
> +{
> + json_object *jobj = NULL;
> + virJSONValue *ret = NULL;
> + json_tokener *tok = NULL;
> + enum json_tokener_error jerr;
> + int jsonflags = JSON_TOKENER_STRICT | JSON_TOKENER_VALIDATE_UTF8;
> +
> + VIR_DEBUG("string=%s", jsonstring);
> +
> + tok = json_tokener_new();
> + json_tokener_set_flags(tok, jsonflags);
> + jobj = json_tokener_parse_ex(tok, jsonstring, strlen(jsonstring));
> + jerr = json_tokener_get_error(tok);
> + if (jerr != json_tokener_success) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + "%s", json_tokener_error_desc(jerr));
> + goto cleanup;
> + }
> + ret = virJSONValueFromJsonC(jobj);
> +
> + cleanup:
> + VIR_DEBUG("result=%p", ret);
So this effectively will always be a pointer except when the
json_tokener_parse_ex returned error, but in that case we'd already
report an error. Is it really needed?
> + json_object_put(jobj);
> + json_tokener_free(tok);
> + return ret;
> +}
> +
> +static json_object *
> +virJSONValueToJsonC(virJSONValue *object)
> +{
> + json_object *ret = NULL;
> + size_t i;
> +
> + VIR_DEBUG("object=%p type=%d", object, object->type);
^^ this is imo more useful.
> +
> + switch (object->type) {
> + case VIR_JSON_TYPE_OBJECT:
> + // constant key?
c99 comment.
> + ret = json_object_new_object();
> + for (i = 0; i < object->data.object.npairs; i++) {
> + json_object *child = virJSONValueToJsonC(object->data.object.pairs[i].value);
> + json_object_object_add(ret, object->data.object.pairs[i].key, child);
> + }
> + return ret;
> + case VIR_JSON_TYPE_ARRAY:
> + /* json_object_new_array_ext was introduced in json-c 0.16 */
> +# if JSON_C_VERSION_NUM < ((0 << 16) | (16 << 8))
> + ret = json_object_new_array();
> +# else
> + ret = json_object_new_array_ext(object->data.array.nvalues);
> +# endif
> + for (i = 0; i < object->data.array.nvalues; i++) {
> + json_object_array_add(ret,
> + virJSONValueToJsonC(object->data.array.values[i]));
> + }
> + return ret;
> +
> + 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. */
> + return json_object_new_double_s(299792458, object->data.number);
oof. Shouldn't we at least attempt to format the number into a double?
I understand that you're using this to allow for the exact
representation and that a cannary value might ease debugging but this
looks a bit weird. At least add a comment explaining that it's a cannary
and a hack so that json-c will use our representation.
> + }
> + case VIR_JSON_TYPE_BOOLEAN:
> + return json_object_new_boolean(object->data.boolean);
> +
> + case VIR_JSON_TYPE_NULL:
> + default:
> + return NULL;
> + }
> + return NULL;
> +}
> +
> +
> +int
> +virJSONValueToBuffer(virJSONValue *object,
> + virBuffer *buf,
> + bool pretty)
> +{
> + json_object *jobj = NULL;
> + const char *str;
> + size_t len;
> + int ret = -1;
> + int jsonflags = JSON_C_TO_STRING_NOSLASHESCAPE;
> +
> + VIR_DEBUG("object=%p", object);
> +
> + if (pretty)
> + jsonflags |= JSON_C_TO_STRING_PRETTY | JSON_C_TO_STRING_SPACED;
> +
> + jobj = virJSONValueToJsonC(object);
> + if (!jobj) {
So here you check the return value of 'virJSONValueToJsonC' but none of
the other calls in this function do that.
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to convert virJSONValue to json-c data"));
> + goto cleanup;
> + }
> +
> + str = json_object_to_json_string_length(jobj, jsonflags, &len);
> +
> + virBufferAdd(buf, str, len);
> + if (pretty)
> + virBufferAddLit(buf, "\n");
> + ret = 0;
> +
> + cleanup:
> + json_object_put(jobj);
> + return ret;
> +}
> +
Two lines.
> +#elif WITH_YAJL
Looks good, but I want to understand the error reporting discrepancy
I've pointed out above before giving r-b.
