Re: [dpdk-dev] [PATCH v6 2/3] test/test_telemetry_data: add unit tests for data to JSON

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jul 20, 2020 at 12:19:06PM +0100, Ciara Power wrote:
> From: Louise Kilheeney <louise.kilheeney@intel.com>
> 
> This patch adds tests for verifying telemetry data structures are
> converted to JSON as expected. Both flat and recursive data structures
> are tested, for all possible value types.
> 
> The app connects to the telemetry socket as a client, and registers one
> command with a corresponding callback function. Each time the callback
> function is called, it copies a global data variable to the data pointer
> passed in by telemetry.
> When a test case is run, the test case function builds up the global
> data variable with the relevant data types, and the expected json string
> output which should be generated from that. The 'test_output()' function
> is used to trigger the callback and ensure the actual output matches
> that expected.
> 
> Signed-off-by: Louise Kilheeney <louise.kilheeney@intel.com>
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> 
> ---
> v6:
>   - Fixed FreeBSD build error.
>   - Added comments and expanded commit log.
>   - Add loop to call test cases stored in a list.
> ---

Thanks, this is a good improvement. Apologies but a few more comments below
that I missed in the previous version.

/Bruce

>  app/test/Makefile              |   1 +
>  app/test/meson.build           |   5 +-
>  app/test/test_telemetry_data.c | 375 +++++++++++++++++++++++++++++++++
>  3 files changed, 379 insertions(+), 2 deletions(-)
>  create mode 100644 app/test/test_telemetry_data.c
> 
> diff --git a/app/test/Makefile b/app/test/Makefile
> index f4065271e..1cb64089c 100644
> --- a/app/test/Makefile
> +++ b/app/test/Makefile
> @@ -145,6 +145,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_LPM) += test_lpm6.c
>  SRCS-$(CONFIG_RTE_LIBRTE_LPM) += test_lpm6_perf.c
>  
>  SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += test_telemetry_json.c
> +SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += test_telemetry_data.c
>  
>  SRCS-y += test_debug.c
>  SRCS-y += test_errno.c
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 786a21397..4a72fe5b6 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -170,6 +170,7 @@ test_deps = ['acl',
>  	'ring',
>  	'security',
>  	'stack',
> +	'telemetry',
>  	'timer'
>  ]
>  
> @@ -345,8 +346,8 @@ if dpdk_conf.has('RTE_LIBRTE_SKELETON_EVENTDEV_PMD')
>  	test_deps += 'pmd_skeleton_event'
>  endif
>  if dpdk_conf.has('RTE_LIBRTE_TELEMETRY')
> -	test_sources += 'test_telemetry_json.c'
> -	fast_tests += [['telemetry_json_autotest', true]]
> +	test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
> +	fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
>  endif
>  
>  # The following linkages of drivers are required because
> diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> new file mode 100644
> index 000000000..f95054433
> --- /dev/null
> +++ b/app/test/test_telemetry_data.c
> @@ -0,0 +1,375 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Intel Corporation
> + */
> +
> +#include <glob.h>

While the python script for testing telemetry does use glob to find the
socket, in this case I think it's unnecessary since we know we are in the
process that created the listening socket, so we can just connect straight
to it.

> +#include <string.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +#include <limits.h>
> +
> +#include <rte_eal.h>
> +#include <rte_common.h>
> +#include <rte_telemetry.h>
> +#include <rte_string_fns.h>
> +
> +#include "test.h"
> +#include "telemetry_data.h"
> +
> +#define TELEMETRY_VERSION "v2"
> +#define REQUEST_CMD "/test"
> +#define BUF_SIZE 1024
> +#define TEST_OUTPUT(exp) test_output(__func__, exp)
> +
> +static struct rte_tel_data response_data;
> +static int sock;
> +
> +/*
> + * This function is the callback registered with Telemetry to be used when
> + * the /test command is requested. This callback returns the global data built
> + * up by the individual test cases.
> + */
> +static int
> +test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
> +		struct rte_tel_data *d)
> +{
> +	*d = response_data;
> +	return 0;
> +}
> +
> +/*
> + * This function is called by each test case function. It communicates with
> + * the telemetry socket by requesting the /test command, and reading the
> + * response. The expected response is passed in by the test case function,
> + * and is compared to the actual response received from Telemetry.
> + */
> +static int
> +test_output(const char *func_name, const char *expected)
> +{
> +	int bytes;
> +	char buf[BUF_SIZE * 16];
> +	if (write(sock, REQUEST_CMD, strlen(REQUEST_CMD)) < 0) {
> +		printf("%s: Error with socket write - %s\n", __func__,
> +				strerror(errno));
> +		return -1;
> +	}
> +	bytes = read(sock, buf, sizeof(buf));
> +	if (bytes < 0) {
> +		printf("%s: Error with socket read - %s\n", __func__,
> +				strerror(errno));
> +		return -1;
> +	}
> +	buf[bytes] = '\0';

Need to check here for overflow. If bytes == BUF_SIZE * 16, you'll
overflow.

> +	printf("%s: buf = '%s', expected = '%s'\n", func_name, buf, expected);
> +	return strncmp(expected, buf, sizeof(buf));
> +}
> +
> +static int
> +test_dict_with_array_int_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_INT_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 5; i++) {
> +		rte_tel_data_add_array_int(child_data, i);
> +		rte_tel_data_add_array_int(child_data2, i);
> +	}
> +
> +	rte_tel_data_add_dict_container(&response_data, "dict_0",
> +	 child_data, 0);
> +	rte_tel_data_add_dict_container(&response_data, "dict_1",
> +	 child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4],\"dict_1\":[0,1,2,3,4]}}");
> +}
> +
> +static int
> +test_array_with_array_int_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_INT_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> +
> +	for (i = 0; i < 5; i++) {
> +		rte_tel_data_add_array_int(child_data, i);
> +		rte_tel_data_add_array_int(child_data2, i);
> +	}
> +	rte_tel_data_add_array_container(&response_data, child_data, 0);
> +	rte_tel_data_add_array_container(&response_data, child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
> +}
> +
> +static int
> +test_case_array_int(void)
> +{
> +	int i;
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_INT_VAL);
> +	for (i = 0; i < 5; i++)
> +		rte_tel_data_add_array_int(&response_data, i);
> +	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
> +}
> +
> +static int
> +test_case_add_dict_int(void)
> +{
> +	int i = 0;
> +	char name_of_value[8];
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 5; i++) {
> +		sprintf(name_of_value, "dict_%d", i);
> +		rte_tel_data_add_dict_int(&response_data, name_of_value, i);
> +	}
> +
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}}");
> +}
> +
> +static int
> +test_case_array_string(void)
> +{
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
> +	rte_tel_data_add_array_string(&response_data, "aaaa");
> +	rte_tel_data_add_array_string(&response_data, "bbbb");
> +	rte_tel_data_add_array_string(&response_data, "cccc");
> +	rte_tel_data_add_array_string(&response_data, "dddd");
> +	rte_tel_data_add_array_string(&response_data, "eeee");
> +
> +	return TEST_OUTPUT("{\"/test\":[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\",\"eeee\"]}");
> +}
> +
> +static int
> +test_case_add_dict_string(void)
> +{
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	rte_tel_data_add_dict_string(&response_data, "dict_0", "aaaa");
> +	rte_tel_data_add_dict_string(&response_data, "dict_1", "bbbb");
> +	rte_tel_data_add_dict_string(&response_data, "dict_2", "cccc");
> +	rte_tel_data_add_dict_string(&response_data, "dict_3", "dddd");
> +
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":\"aaaa\",\"dict_1\":\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}}");
> +}

Although these are single strings, they are not user output, and so you can
split them across lines to limit line length.

> +
> +
> +static int
> +test_dict_with_array_string_values(void)
> +{
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_STRING_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	rte_tel_data_add_array_string(child_data, "aaaa");
> +	rte_tel_data_add_array_string(child_data2, "bbbb");
> +
> +	rte_tel_data_add_dict_container(&response_data, "dict_0",
> +	 child_data, 0);
> +	rte_tel_data_add_dict_container(&response_data, "dict_1",
> +	 child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}");
> +}
> +
> +static int
> +test_array_with_array_string_values(void)
> +{
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_STRING_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> +
> +	rte_tel_data_add_array_string(child_data, "aaaa");
> +	rte_tel_data_add_array_string(child_data2, "bbbb");
> +
> +	rte_tel_data_add_array_container(&response_data, child_data, 0);
> +	rte_tel_data_add_array_container(&response_data, child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":[[\"aaaa\"],[\"bbbb\"]]}");
> +}
> +
> +static int
> +test_case_array_u64(void)
> +{
> +	int i;
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_U64_VAL);
> +	for (i = 0; i < 5; i++)
> +		rte_tel_data_add_array_u64(&response_data, i);
> +	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
> +}
> +
> +static int
> +test_case_add_dict_u64(void)
> +{
> +	int i = 0;
> +	char name_of_value[8];
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 5; i++) {
> +		sprintf(name_of_value, "dict_%d", i);
> +		rte_tel_data_add_dict_u64(&response_data, name_of_value, i);
> +	}
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}}");
> +}
> +
> +static int
> +test_dict_with_array_u64_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_U64_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 10; i++) {
> +		rte_tel_data_add_array_u64(child_data, i);
> +		rte_tel_data_add_array_u64(child_data2, i);
> +	}
> +
> +	rte_tel_data_add_dict_container(&response_data, "dict_0",
> +	 child_data, 0);
> +	rte_tel_data_add_dict_container(&response_data, "dict_1",
> +	 child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}}");
> +}
> +
> +static int
> +test_array_with_array_u64_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_U64_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
> +
> +	memset(&response_data, 0, sizeof(response_data));
> +	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> +
> +	for (i = 0; i < 5; i++) {
> +		rte_tel_data_add_array_u64(child_data, i);
> +		rte_tel_data_add_array_u64(child_data2, i);
> +	}
> +	rte_tel_data_add_array_container(&response_data, child_data, 0);
> +	rte_tel_data_add_array_container(&response_data, child_data2, 0);
> +
> +	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
> +}
> +
> +static int
> +connect_to_socket(void)
> +{
> +	char file_pattern[PATH_MAX];
> +	char buf[BUF_SIZE];
> +	glob_t globbuf;
> +	int sock, bytes;
> +	struct sockaddr_un telem_addr;
> +
> +	snprintf(file_pattern, sizeof(file_pattern),
> +			"%s/dpdk_telemetry.%s",	rte_eal_get_runtime_dir(),
> +			TELEMETRY_VERSION);
> +	if (glob(file_pattern, 0, NULL, &globbuf) != 0) {
> +		printf("\n%s: Error finding socket file path: %s\n", __func__,
> +				strerror(errno));
> +		globfree(&globbuf);
> +		return -1;
> +	}
> +

The path you create here should be the exact path that you need, no
globbing should be necessary, which I think should simplify the code a bit.

> +	sock = socket(AF_UNIX, SOCK_SEQPACKET, 0);
> +	if (sock < 0) {
> +		printf("\n%s: Error creating socket: %s\n", __func__,
> +				strerror(errno));
> +		return -1;
> +	}
> +	telem_addr.sun_family = AF_UNIX;
> +	strlcpy(telem_addr.sun_path, globbuf.gl_pathv[0],
> +			sizeof(telem_addr.sun_path));
> +	if (connect(sock, (struct sockaddr *) &telem_addr,
> +			sizeof(telem_addr)) < 0)
> +		printf("\n%s: Error connecting to socket: %s\n", __func__,
> +				strerror(errno));
> +
> +	bytes = read(sock, buf, sizeof(buf));
> +	if (bytes < 0) {
> +		printf("%s: Error with socket read - %s\n", __func__,
> +				strerror(errno));
> +		return -1;
> +	}
> +	buf[bytes] = '\0';

Again, watch for overflow when you hit the exact output limit.

> +	printf("\n%s: %s\n", __func__, buf);
> +	globfree(&globbuf);
> +	return sock;
> +}
> +
> +static int
> +test_telemetry_data(void)
> +{
> +	typedef int (*test_case)(void);
> +	unsigned int i = 0;
> +
> +	sock = connect_to_socket();
> +	if (sock <= 0)
> +		return -1;
> +
> +	test_case test_cases[] = {test_case_array_string,
> +			test_case_array_int, test_case_array_u64,
> +			test_case_add_dict_int, test_case_add_dict_u64,
> +			test_case_add_dict_string,
> +			test_dict_with_array_int_values,
> +			test_dict_with_array_u64_values,
> +			test_dict_with_array_string_values,
> +			test_array_with_array_int_values,
> +			test_array_with_array_u64_values,
> +			test_array_with_array_string_values };
> +
> +	rte_telemetry_register_cmd(REQUEST_CMD, test_cb, "Test");
> +	for (i = 0; i < RTE_DIM(test_cases); i++) {
> +		if (test_cases[i]() != 0) {
> +			close(sock);
> +			return -1;
> +		}
> +	}

Thanks, this is much more readable than the previous version.

> +	close(sock);
> +	return 0;
> +}
> +
> +REGISTER_TEST_COMMAND(telemetry_data_autotest, test_telemetry_data);
> -- 
> 2.17.1
>