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

[Vladimir Davydov]

On Tue, Dec 25, 2018 at 11:53:08AM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/lib/json/json.c b/src/lib/json/json.c
> index c7909fde2..57449a3aa 100644
> --- a/src/lib/json/json.c
> +++ b/src/lib/json/json.c
> @@ -317,6 +317,77 @@ json_path_validate(const char *path, int path_len, int index_base)
>  	return rc;
>  }
>  
> +/**
> + * Helper routine for json_tree_snprint_path to snprintf atomic

atomic?

> + * token. Works the same way as json_tree_snprint_path, read it's
> + * comment for more details.

You could simply copy the description of any other snprint-like function
we have:

/**
 * An snprint-style helper to print an individual token key.
 */

Would be more concise and clear.

> + */
> +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_tree_snprint_path(char *buf, int size, const struct json_token *token,
> +		       const struct json_token *root, int index_base)

root should go first in the argument list, because it goes first in all
other json_tree methods.

> +{
> +	/*
> +	 * Calculate JSON path string length passing 0-buffer in
> +	 * json_token_snprint routine.
> +	 */
> +	int len = 0;
> +	const struct json_token *ptr = token;
> +	while (ptr != root && ptr->type != JSON_TOKEN_END) {
> +		len += json_token_snprint(NULL, 0, ptr, index_base);
> +		ptr = ptr->parent;
> +	}
> +	if (size == 0)
> +		return len;
> +
> +	/*
> +	 * Write to the memory buffer buf as many tokens,
> +	 * starting with the root, as it can fit. If the fragment
> +	 * of the token does not fit into the buffer, it would be
> +	 * truncated.
> +	 */
> +	int pos = len;
> +	char last = '\0';
> +	ptr = token;
> +	while (ptr != root && ptr->type != JSON_TOKEN_END) {
> +		pos -= json_token_snprint(NULL, 0, ptr, index_base);
> +		assert(pos >= 0);
> +		if (pos + 1 < size) {

Why pos + 1? Due to this extra 1, this function will crash if passed a
buffer of length 1.

> +			int rc = json_token_snprint(buf + pos, size - pos, ptr,
> +						    index_base);
> +			if (rc < 0)
> +				return rc;

How can it happen?

> +			/*
> +			 * The json_token_snprint writes a
> +			 * '\0'-terminated string in memory
> +			 * buffer. The first character in
> +			 * token string representation would be
> +			 * overwritten on next cycle iteration.
> +			 * Let's keep it in 'last' variable to
> +			 * restore next time.
> +			 */
> +			buf[pos + rc] = last;

rc can be greater than size-pos, in which case you'll write beyond the
supplied buffer.

> +			last = buf[pos];
> +		}
> +		ptr = ptr->parent;
> +	}
> +	assert(buf[MIN(len, size - 1)] == '\0');
> +	return len;
> +}
> +
>  #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..bed20536d 100644
> --- a/src/lib/json/json.h
> +++ b/src/lib/json/json.h
> @@ -253,6 +253,30 @@ 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 a JSON tree path path between root and token items to
> + * memory buffer buf, at most size bytes (including the null byte
> + * used to end output to strings).
> + * The parent argument may omitted via passing NULL value. It this

There's no 'parent' argument. You mean 'root'? But we always require the
caller to pass the root to all other JSON tree methods. Why make this
one different?

> + * case the whole JSON path would be printed.

> + * When routine is called with size=0, return value (as always)
> + * is the number of characters that would have been written in
> + * case the output string had been large enough.

This paragraph is pointless, because it directly follows from the next
paragraph.

> + *
> + * Upon successful return, routine returns the number of
> + * characters printed (excluding the null byte used to end output
> + * to strings). If the output was truncated due the size limit,
> + * then the return value is the number of characters (excluding
> + * the terminating null byte) which would have been written to
> + * the final string if enough space had been available.
> + * Thus, a return value of size or more means that the output
> + * was truncated.
> + * If an output error is encountered, a negative value is
> + * returned.

What error are you talking about? This function isn't supposed to fail.
Looks like you blindly copied snprintf() man page.

Instead you could simply write something like:

  An snprint-style function to print path to a token in a JSON tree.
  The root node doesn't necessarily have to be the tree root - it may be
  any ascendant of the given token, in which case a path from the given
  ascendant to the token will be printed.

Everyone who wants to know what snprintf() returns can read its manual
page.

> + */
> +int
> +json_tree_snprint_path(char *buf, int size, const struct json_token *token,
> +		       const struct json_token *root, int index_base);
>  /**
>   * Initialize a new empty JSON tree.
>   *
> diff --git a/test/unit/json.c b/test/unit/json.c
> index e553ff23c..122e605cf 100644
> --- a/test/unit/json.c
> +++ b/test/unit/json.c
> @@ -438,16 +438,81 @@ test_path_cmp()
>  	footer();
>  }
>  
> +void
> +test_path_snprintf()
> +{
> +	header();
> +	plan(24);
> +
> +	struct json_tree tree;
> +	int rc = json_tree_create(&tree);
> +	fail_if(rc != 0);
> +	struct test_struct records[5];
> +	const char *path = "[1][20][\"file\"][8]";
> +	int path_len = strlen(path);
> +
> +	int records_idx = 0;
> +	struct test_struct *node, *node_tmp;
> +	node = test_add_path(&tree, path, path_len, records, &records_idx);
> +	fail_if(&node->node != &records[3].node);
> +
> +	/* Test size estimation. */
> +	char buff[64];
> +	int len = json_tree_snprint_path(NULL, 0, &node->node, NULL, INDEX_BASE);
> +	is(len, path_len, "estimate path length: have %d expected %d",
> +	   len, path_len);
> +	len = json_tree_snprint_path(NULL, 0, &node->node, &records[0].node,
> +				     INDEX_BASE);
> +	is(len, 15, "estimate path part length: have %d expected %d", len, 15);
> +
> +	len = json_tree_snprint_path(NULL, 0, &node->node, &records[1].node,
> +				     INDEX_BASE);
> +	is(len, 11, "estimate path part length: have %d expected %d", len, 11);
> +	len = json_tree_snprint_path(NULL, 0, &node->node, &records[2].node,
> +				     INDEX_BASE);
> +	is(len, 3, "estimate path part length: have %d expected %d", len, 3);
> +
> +	/* Test truncate. */
> +	for (int i = path_len + 1; i > strrchr(path, '[') - path - 2; i--) {
> +		len = json_tree_snprint_path(buff, i + 1, &node->node, NULL,
> +					     INDEX_BASE);
> +		is(len, path_len, "correct char count: have %d expected %d",
> +		   len, path_len);
> +		len = MIN(len, i);
> +		is(memcmp(buff, path, len), 0,
> +		   "correct string fragment: have \"%s\", expected \"%.*s\"",
> +		   buff, len, path);
> +		is(buff[len], '\0', "terminating '\\0' at appropriate place");
> +	}

This is difficult for understanding. Why can't you simply choose a few
values of the buffer length and check that the function works for them
as expected? And I don't see any point special-casing NULL buffer here -
after all there's nothing special about it - it should execute the same
code as any other buffer length.

> +
> +	/* Test path fragment snprintf. */
> +	len = json_tree_snprint_path(buff, 16, &node->node, &records[0].node,
> +				     INDEX_BASE);
> +	is(memcmp(buff, path + 3, len), 0,
> +	   "correct partial JSON path: have \"%s\" expected \"%s\"", buff,
> +	   path + 3);
> +	is(buff[len], '\0', "terminating '\\0' at appropriate place");
> +
> +	json_tree_foreach_entry_safe(node, &tree.root, struct test_struct,
> +				     node, node_tmp)
> +		json_tree_del(&tree, &node->node);
> +	json_tree_destroy(&tree);
> +
> +	check_plan();
> +	footer();
> +}