Re: [PATCH v1 3/5] lib: introduce json_token_path_snprint

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Sun, Dec 23, 2018 at 03:40:38PM +0300, Kirill Shcherbatov wrote:
> Implemented a helper function for getting a path to a json_token
> in a json_tree. When routine is called with size=0, return value
> (as always) as the number of characters that would have been
> written in case the output string has been large enough.
> We will use this function for reporting tuple format violations
> in further patches.
> 
> Needed for #1012
> ---
>  src/lib/json/json.c   |  46 +++++++++++++++++
>  src/lib/json/json.h   |  13 +++++
>  test/unit/json.c      |  20 +++++++-
>  test/unit/json.result | 113 ++++++++++++++++++++++++------------------
>  4 files changed, 144 insertions(+), 48 deletions(-)
> 
> diff --git a/src/lib/json/json.c b/src/lib/json/json.c
> index c7909fde2..e933abe68 100644
> --- a/src/lib/json/json.c
> +++ b/src/lib/json/json.c
> @@ -317,6 +317,52 @@ json_path_validate(const char *path, int path_len, int index_base)
>  	return rc;
>  }
>  

Comment is missing.

> +static int
> +json_token_snprint(char *buf, int size, const struct json_token *token,
> +		   int index_base)
> +{
> +	enum json_token_type type = token->type;
> +	assert(type == JSON_TOKEN_NUM || type == JSON_TOKEN_STR);
> +	int len = 0;
> +	if (type == JSON_TOKEN_NUM)
> +		len = snprintf(buf, size, "[%d]", token->num + index_base);
> +	else if (type == JSON_TOKEN_STR)
> +		len = snprintf(buf, size, "[\"%.*s\"]", token->len, token->str);
> +	return len;
> +}
> +
> +int
> +json_token_path_snprint(char *buf, int size, const struct json_token *token,
> +		       int index_base)

The function name is confusing: it implies that the function is
applicable to any token while in fact it's only relevant to tokens
linked in a tree. What about json_tree_snprint_path()?

Also, I think that this function should take not only the token itself,
but also the tree root as it would be more flexible that way. We may
need this in future, e.g. for properly reporting tuple field names
specified in the tuple format dictionary.

> +{
> +	int ret = 0;
> +	const struct json_token *ptr = token;
> +	while (ptr->type != JSON_TOKEN_END) {

It would be nice to see some comments here, explaining what the two
loops comprising this function do.

> +		ret += json_token_snprint(NULL, 0, ptr, index_base);
> +		ptr = ptr->parent;
> +	}
> +	if (size == 0)
> +		return ret;
> +
> +	int len = ret;

Looking at how this variable is used, I can say that it's named
improperly: I'd rather call it 'pos'. And I'd also rename 'ret' to
'len', because it denotes the length of the output string, actually.

> +	char last = '\0';
> +	ret = MIN(ret, size - 1);
> +	ptr = token;
> +	while (ptr->type != JSON_TOKEN_END) {
> +		len -= json_token_snprint(NULL, 0, ptr, index_base);
> +		assert(len >= 0);
> +		if (len + 1 < size) {
> +			int rc = json_token_snprint(buf + len, size - len, ptr,
> +						    index_base);
> +			buf[len + rc] = last;

Please add a comment explaining why you need to restore the last
character in the output here.

> +			last = buf[len];
> +		}
> +		ptr = ptr->parent;
> +	}
> +	buf[ret] = '\0';
> +	return ret;
> +}
> +
>  #define MH_SOURCE 1
>  #define mh_name _json
>  #define mh_key_t struct json_token *
> diff --git a/src/lib/json/json.h b/src/lib/json/json.h
> index 7ce10ce2c..5df5a1efb 100644
> --- a/src/lib/json/json.h
> +++ b/src/lib/json/json.h
> @@ -253,6 +253,19 @@ json_path_cmp(const char *a, int a_len, const char *b, int b_len,
>  int
>  json_path_validate(const char *path, int path_len, int index_base);
>  
> +/**
> + * Write path to JSON tree token to memory buffer buf, at most
> + * size bytes (including the null byte used to end output to\

What's '\' here for?

> + * strings).
> + * When routine is called with size=0, return value
> + * (as always) as the number of characters that would have been

s/as/is

> + * written in case the output string has been large enough.

s/has/had

> + * Return the number of characters printed (excluding '\0').
> + */
> +int
> +json_token_path_snprint(char *buf, int size, const struct json_token *token,
> +			int index_base);
> +

Please carefully read snprintf() manual page and fix this function so
that it behaves in the same way, i.e. always returns the number of
characters that would have been printed if there had been enough space
in the buffer.


>  /**
>   * Initialize a new empty JSON tree.
>   *
> diff --git a/test/unit/json.c b/test/unit/json.c
> index e553ff23c..7cfea161f 100644
> --- a/test/unit/json.c
> +++ b/test/unit/json.c
> @@ -211,7 +211,7 @@ void
>  test_tree()
>  {
>  	header();
> -	plan(54);
> +	plan(73);
>  
>  	struct json_tree tree;
>  	int rc = json_tree_create(&tree);
> @@ -265,6 +265,24 @@ test_tree()
>  					   struct test_struct, node);
>  	is(node, NULL, "lookup unregistered path '%s'", path_unregistered);
>  
> +	/* Test path snprintf. */
> +	char buff[64];
> +	int len = json_token_path_snprint(NULL, 0, &records[5].node, INDEX_BASE);
> +	int path4_copy_len = strlen(path4_copy);
> +	is(len, path4_copy_len, "estimate path length: have %d expected %d",
> +	   len, path4_copy_len);
> +	for (int i = path4_copy_len + 1;
> +	     i > strrchr(path4_copy, '[') - path4_copy - 2; i--) {
> +		int rc = json_token_path_snprint(buff, i + 1, &records[5].node,
> +						 INDEX_BASE);
> +		len = MIN(path4_copy_len, i);
> +		is(rc, len, "correct char count");
> +		is(memcmp(buff, path4_copy, len) == 0, true,
> +		   "correct string fragment: have \"%s\", expected \"%.*s\"",
> +		   buff, len, path4_copy);
> +		is(buff[len], '\0', "terminating '\\0' at appropriate place");
> +	}
> +
>  	/* Test iterators. */
>  	struct json_token *token = NULL, *tmp;
>  	const struct json_token *tokens_preorder[] =
> diff --git a/test/unit/json.result b/test/unit/json.result
> index a17451099..4ca6cf4b5 100644
> --- a/test/unit/json.result
> +++ b/test/unit/json.result
> @@ -101,7 +101,7 @@ ok 1 - subtests
>  ok 2 - subtests
>  	*** test_errors: done ***
>  	*** test_tree ***
> -    1..54
> +    1..73
>      ok 1 - add path '[1][10]'
>      ok 2 - add path '[1][20].file'
>      ok 3 - add path '[1][20].file[2]'
> @@ -110,52 +110,71 @@ ok 2 - subtests
>      ok 6 - lookup path '[1][10]'
>      ok 7 - lookup path '[1][20].file'
>      ok 8 - lookup unregistered path '[1][3]'
> -    ok 9 - test foreach pre order 0: have 0 expected of 0
> -    ok 10 - test foreach pre order 1: have 1 expected of 1
> -    ok 11 - test foreach pre order 2: have 2 expected of 2
> -    ok 12 - test foreach pre order 3: have 3 expected of 3
> -    ok 13 - test foreach pre order 4: have 4 expected of 4
> -    ok 14 - test foreach pre order 5: have 5 expected of 5
> -    ok 15 - records iterated count 6 of 6
> -    ok 16 - test foreach post order 0: have 1 expected of 1
> -    ok 17 - test foreach post order 1: have 4 expected of 4
> -    ok 18 - test foreach post order 2: have 5 expected of 5
> -    ok 19 - test foreach post order 3: have 3 expected of 3
> -    ok 20 - test foreach post order 4: have 2 expected of 2
> -    ok 21 - test foreach post order 5: have 0 expected of 0
> -    ok 22 - records iterated count 6 of 6
> -    ok 23 - test foreach safe order 0: have 1 expected of 1
> -    ok 24 - test foreach safe order 1: have 4 expected of 4
> -    ok 25 - test foreach safe order 2: have 5 expected of 5
> -    ok 26 - test foreach safe order 3: have 3 expected of 3
> -    ok 27 - test foreach safe order 4: have 2 expected of 2
> -    ok 28 - test foreach safe order 5: have 0 expected of 0
> -    ok 29 - records iterated count 6 of 6
> -    ok 30 - test foreach entry pre order 0: have 0 expected of 0
> -    ok 31 - test foreach entry pre order 1: have 1 expected of 1
> -    ok 32 - test foreach entry pre order 2: have 2 expected of 2
> -    ok 33 - test foreach entry pre order 3: have 3 expected of 3
> -    ok 34 - test foreach entry pre order 4: have 4 expected of 4
> -    ok 35 - test foreach entry pre order 5: have 5 expected of 5
> -    ok 36 - records iterated count 6 of 6
> -    ok 37 - test foreach entry post order 0: have 1 expected of 1
> -    ok 38 - test foreach entry post order 1: have 4 expected of 4
> -    ok 39 - test foreach entry post order 2: have 5 expected of 5
> -    ok 40 - test foreach entry post order 3: have 3 expected of 3
> -    ok 41 - test foreach entry post order 4: have 2 expected of 2
> -    ok 42 - test foreach entry post order 5: have 0 expected of 0
> -    ok 43 - records iterated count 6 of 6
> -    ok 44 - max_child_index 7 expected of 7
> -    ok 45 - max_child_index 1 expected of 1
> -    ok 46 - max_child_index -1 expected of -1
> -    ok 47 - lookup removed path '[1][20].file[2]'
> -    ok 48 - lookup removed path '[1][20].file[8]'
> -    ok 49 - lookup path was not corrupted '[1][20].file'
> -    ok 50 - test foreach entry safe order 0: have 1 expected of 1
> -    ok 51 - test foreach entry safe order 1: have 3 expected of 3
> -    ok 52 - test foreach entry safe order 2: have 2 expected of 2
> -    ok 53 - test foreach entry safe order 3: have 0 expected of 0
> -    ok 54 - records iterated count 4 of 4
> +    ok 9 - estimate path length: have 18 expected 18
> +    ok 10 - correct char count
> +    ok 11 - correct string fragment: have "[1][20]["file"][8]", expected "[1][20]["file"][8]"
> +    ok 12 - terminating '\0' at appropriate place
> +    ok 13 - correct char count
> +    ok 14 - correct string fragment: have "[1][20]["file"][8]", expected "[1][20]["file"][8]"
> +    ok 15 - terminating '\0' at appropriate place
> +    ok 16 - correct char count
> +    ok 17 - correct string fragment: have "[1][20]["file"][8", expected "[1][20]["file"][8"
> +    ok 18 - terminating '\0' at appropriate place
> +    ok 19 - correct char count
> +    ok 20 - correct string fragment: have "[1][20]["file"][", expected "[1][20]["file"]["
> +    ok 21 - terminating '\0' at appropriate place
> +    ok 22 - correct char count
> +    ok 23 - correct string fragment: have "[1][20]["file"]", expected "[1][20]["file"]"
> +    ok 24 - terminating '\0' at appropriate place
> +    ok 25 - correct char count
> +    ok 26 - correct string fragment: have "[1][20]["file"", expected "[1][20]["file""
> +    ok 27 - terminating '\0' at appropriate place

Please add the new test case to the end of the file so that it doesn't
inflate the diff by shifting the output of existing test cases.

> +    ok 28 - test foreach pre order 0: have 0 expected of 0
> +    ok 29 - test foreach pre order 1: have 1 expected of 1
> +    ok 30 - test foreach pre order 2: have 2 expected of 2
> +    ok 31 - test foreach pre order 3: have 3 expected of 3
> +    ok 32 - test foreach pre order 4: have 4 expected of 4
> +    ok 33 - test foreach pre order 5: have 5 expected of 5