Re: [PATCH v2 2/3] json: introduce JSON module

[Denis Kenzior <denkenz at gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 12854 bytes --]

Hi James,

On 12/8/21 3:36 PM, James Prestwood wrote:
> This is a minimal wrapper around jsmn.h to make things a bit easier
> for iterating through a JSON object.
> 
> An iterator can be created/destroyed with json_iter_{new,free} and
> values can be parsed from this iterator using json_iter_parse.
> 

Have you considered using an intermediate object to hold the bits that need to 
be opaque?  That way json_iter objects could be public and held on the stack. 
Would make things easier to use / simpler to implement.

So roughly:

struct json_contents;

struct json_iter {
	struct json_contents *contents;
	int begin;
	int end;
};

also you could possibly track parent objects if needed.

> Arguments to json_iter_parse should use JSON_MANDATORY/JSON_OPTIONAL
> macros, and terminate the argument list with JSON_FLAG_INVALID.
> 
> The value pointer in the MANDATORY/OPTIONAL macros can be NULL, in
> which case the presence of the key/value is checked but no data is
> returned out.
> 
> Currently only JSON_STRING and JSON_OBJECT types are supported. String
> values should be of type struct iovec *, and objects should be of

Ok, I wonder if we could allocate the strings instead of having the caller do this.

> type struct json_iter **. These nested objects can be parsed again
> using json_iter_parse and should not be freed (the original iterator
> keeps track of nested object iterators).

It would be nice if these were simply on the stack to cut down the queue management.

> ---
>   Makefile.am |   1 +
>   src/json.c  | 222 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/json.h  |  84 ++++++++++++++++++++
>   3 files changed, 307 insertions(+)
>   create mode 100644 src/json.c
>   create mode 100644 src/json.h
> 
> v2:
>   * Re-wrote to remove callback style parsing. Instead the expected values
>     are provided to the parser directly.
> 
> diff --git a/Makefile.am b/Makefile.am
> index 7fe4b5c3..1ae7e3c0 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -249,6 +249,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
>   					src/sysfs.h src/sysfs.c \
>   					src/offchannel.h src/offchannel.c \
>   					src/dpp-util.h src/dpp-util.c \
> +					src/json.h src/json.c \
>   					$(eap_sources) \
>   					$(builtin_sources)
>   
> diff --git a/src/json.c b/src/json.c
> new file mode 100644
> index 00000000..9f6c744f
> --- /dev/null
> +++ b/src/json.c
> @@ -0,0 +1,222 @@
> +/*
> + *
> + *  Wireless daemon for Linux
> + *
> + *  Copyright (C) 2013-2019  Intel Corporation. All rights reserved.

Please update this

> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <errno.h>
> +
> +#include <ell/ell.h>
> +
> +#include "src/json.h"
> +
> +#include "shared/jsmn.h"
> +
> +/* Max number of tokens supported. Increase if larger objects are expected */
> +#define JSON_DEFAULT_TOKENS 30
> +
> +#define TOK_LEN(token) (token)->end - (token)->start
> +#define TOK_PTR(json, token) (void *)((json) + (token)->start)
> +
> +struct json_iter {
> +	const char *json;
> +	size_t json_len;
> +	jsmntok_t *tokens;
> +	int tokens_len;
> +	jsmn_parser *p;
> +	struct l_queue *children;
> +	bool root : 1;
> +};
> +
> +static struct json_iter *iter_recurse(struct json_iter *parent,
> +					jsmntok_t *object)
> +{
> +	struct json_iter *iter = l_new(struct json_iter, 1);
> +
> +	iter->json = parent->json;
> +	iter->json_len = parent->json_len;
> +	iter->tokens = object;
> +	iter->children = l_queue_new();
> +
> +	/*
> +	 * Since object->size only indicates the number of keys in an object
> +	 * we cannot easily calculate the number of tokens remaining (for this
> +	 * object) since each object may have nested objects. Set tokens_len to
> +	 * the number of tokens remaining in the overall list after 'object'

This seems brittle.  Can we recursively count the number of tokens for each child?

> +	 */
> +	iter->tokens_len = parent->tokens_len -
> +				((object - parent->tokens) % sizeof(jsmntok_t));
> +
> +	l_queue_push_head(parent->children, iter);
> +
> +	return iter;
> +}
> +
> +struct json_iter *json_iter_new(const char *json, size_t json_len)
> +{
> +	struct json_iter *iter = l_new(struct json_iter, 1);
> +
> +	iter->json = json;
> +	iter->json_len = json_len;
> +	iter->p = l_new(jsmn_parser, 1);

Do you need jsmn_init here?

> +	iter->tokens = l_new(jsmntok_t, JSON_DEFAULT_TOKENS);
> +	iter->root = true;
> +
> +	jsmn_init(iter->p);
> +	iter->tokens_len = jsmn_parse(iter->p, iter->json, iter->json_len,
> +					iter->tokens, JSON_DEFAULT_TOKENS);
> +	if (iter->tokens_len < 0) {
> +		json_iter_free(iter);
> +		return NULL;
> +	}
> +
> +	iter->children = l_queue_new();
> +
> +	return iter;
> +}
> +
> +static void json_iter_destroy(void *data)
> +{
> +	struct json_iter *iter = data;
> +
> +	json_iter_free(iter);
> +}
> +
> +void json_iter_free(struct json_iter *iter)
> +{
> +	if (iter->root) {
> +		l_free(iter->p);
> +		l_free(iter->tokens);
> +	}
> +
> +	l_queue_destroy(iter->children, json_iter_destroy);

You could do:
l_queue_destroy(iter->children,
	(l_queue_destroy_func_t *) json_iter_free);

> +	l_free(iter);
> +}
> +
> +static jsmntok_t *next_key(struct json_iter *iter, jsmntok_t *current)
> +{
> +	jsmntok_t *value;
> +	int i;
> +
> +	/*
> +	 * In theory JSMN should prevent any of these bounds checks from failing

Not very reassuring :)

> +	 */
> +	if (current + 1 > iter->tokens + iter->tokens_len)
> +		return NULL;
> +
> +	value = current + 1;
> +
> +	if (value->size == 0)
> +		return value + 1;

So value->size == 0 implies a basic type?

> + > +	for (i = 0; i < value->size; i++)
> +		value = next_key(iter, value);

So this should be recursively skipping tokens when value is non-basic.  The 
naming is a bit confusing here

> +
> +	return value;

Since you're returning 'value', but the reader expects a 'key'.

> +}
> +
> +/*
> + * Checks that the keys value (t + 1) is in bounds, the value type matches,
> + * key length matches, and key string match
> + */
> +#define TOKEN_MATCHES(iter, t, key, type) \
> +	(t) + 1 < iter->tokens + iter->tokens_len && \
> +	(jsmntype_t)(type) == ((t) + 1)->type && \
> +	TOK_LEN((t)) == (int)strlen((key)) && \
> +	!memcmp(TOK_PTR((iter)->json, (t)), (key), TOK_LEN((t)))
> +
> +bool json_iter_parse(struct json_iter *iter, ...)
> +{
> +	va_list va;
> +	int i;
> +	int num = iter->tokens->size;
> +	jsmntok_t *next;
> +	struct iovec *sval;
> +	struct json_iter **oval;
> +
> +	/* Empty object {} */
> +	if (iter->tokens_len < 1)
> +		return false;
> +
> +	va_start(va, iter);
> +
> +	while (true) {
> +		enum json_flag flag;
> +		char *key;
> +		enum json_type type;
> +		void *value;
> +		jsmntok_t *v = NULL;
> +
> +		flag = va_arg(va, enum json_flag);
> +
> +		if (flag == JSON_FLAG_INVALID)
> +			break;
> +
> +		key = va_arg(va, char *);
> +		type = va_arg(va, enum json_type);
> +		value = va_arg(va, void *);
> +
> +		next = iter->tokens + 1;
> +
> +		/* Iterate over this objects keys */
> +		for (i = 0; i < num; i++) {
> +			if (TOKEN_MATCHES(iter, next, key, type)) {
> +				v = next + 1;
> +				break;

What if the key matches but the type doesn't?  Shouldn't we bail early?

> +			}
> +
> +			next = next_key(iter, next);
> +			if (!next)
> +				return false;

May not be as big of a deal in this particular case, but this approach is 
side-effecting on failure.  You could parse the object part of the way through, 
then fail to find a key and bail, but you may have assigned stuff along the way.

> +		}
> +
> +		if (flag == JSON_FLAG_MANDATORY && !v)
> +			return false;
> +
> +		switch (type) {
> +		case JSON_STRING:
> +			if (!value)
> +				continue;
> +
> +			sval = value;
> +			sval->iov_base = v ? TOK_PTR(iter->json, v) : NULL;
> +			sval->iov_len = v ? TOK_LEN(v) : 0;
> +
> +			break;
> +		case JSON_OBJECT:
> +			if (!value)
> +				continue;
> +
> +			oval = value;
> +			*oval = v ? iter_recurse(iter, v) : NULL;
> +
> +			break;
> +		default:
> +			return false;
> +		}
> +	}
> +
> +	va_end(va);
> +
> +	return true;
> +}
> diff --git a/src/json.h b/src/json.h
> new file mode 100644
> index 00000000..202445ee
> --- /dev/null
> +++ b/src/json.h
> @@ -0,0 +1,84 @@
> +/*
> + *
> + *  Wireless daemon for Linux
> + *
> + *  Copyright (C) 2021  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +struct json_iter;
> +
> +/*
> + * Identical to JSMN types
> + */
> +enum json_type {
> +	JSON_UNDEFINED = 0,
> +	JSON_OBJECT = 1 << 0,
> +	JSON_ARRAY = 1 << 1,
> +	JSON_STRING = 1 << 2,
> +	JSON_PRIMITIVE = 1 << 3
> +};
> +
> +enum json_flag {
> +	JSON_FLAG_INVALID = 0,
> +	JSON_FLAG_MANDATORY = 1,
> +	JSON_FLAG_OPTIONAL = 2,
> +};
> +
> +#define JSON_MANDATORY(key, type, out) \
> +	JSON_FLAG_MANDATORY, (key), (type), (out)
> +
> +#define JSON_OPTIONAL(key, type, out) \
> +	JSON_FLAG_OPTIONAL, (key), (type), (out)
> +
> +#define JSON_STR_EQ(iov, s) \
> +	((iov)->iov_len == strlen((s)) && \
> +	!memcmp((iov)->iov_base, (s), (iov)->iov_len))
> +
> +#define JSON_TO_STR(iov) \
> +({ \
> +	char *_tmp = l_malloc((iov)->iov_len + 1); \
> +	memcpy(_tmp, (iov)->iov_base, (iov)->iov_len); \
> +	_tmp[(iov)->iov_len] = '\0'; \
> +	_tmp; \
> +})
> +
> +struct json_iter *json_iter_new(const char *json, size_t json_len);
> +void json_iter_free(struct json_iter *iter);
> +
> +/*
> + * Parse an arbitrary number of key/value pairs from a JSON iterator. Initially
> + * a new JSON iterator should be created with json_iter_new() and, when done,
> + * freed with json_iter_free. Nested objects are also parsed with this function
> + * but these iterators should not be freed and are tracked by the original
> + * iterator.
> + *
> + * Arguments should be specified using JSON_MANDATORY or JSON_OPTIONAL:
> + *
> + * r = json_iter_parse(iter, JSON_MANDATORY("mykey", JSON_STRING, &strvalue),
> + * 			JSON_OPTIONAL("optkey", JSON_STRING, &optvalue));
> + *
> + * String values should be of type struct iovec *
> + * Object values should be of type struct json_iter **
> + *
> + * No other types are supported at this time, and json_iter_parse will fail if
> + * other types are encountered.
> + *
> + * JSON_OPTIONAL string values will have a NULL/0 iov_base/iov_len if they were
> + * not found. JSON_OPTIONAL object value iterators will be NULL if not found.
> + */
> +bool json_iter_parse(struct json_iter *iter, ...);

Just a nit, but it is a bit clearer to start the '...' after either a 
printf-style string or an argument that could carry the sentintel value.  For 
example:

int nl80211_parse_attrs(struct l_genl_msg *msg, int tag, ...);

It isn't much, but gives just a bit extra context for how the API is supposed to 
be used.  So in the case of json_iter_parse it would be enum json_flag.

Though really, I wonder why json_iter_parse doesn't use the sequence of:

(json_type, key, value, flags) ?

Regards,
-Denis