From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id CD99DA0540; Mon, 20 Jul 2020 15:08:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 235A41DBB; Mon, 20 Jul 2020 15:08:27 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 24B121023 for ; Mon, 20 Jul 2020 15:08:23 +0200 (CEST) IronPort-SDR: M1rCWfhfd0J+QyPOM3ZSqGbumrjeSeGzpJCX/v223FwycxsT0Ct7hYWZP3IFUpwuPXnheLHB42 24XIFBKOvBxA== X-IronPort-AV: E=McAfee;i="6000,8403,9687"; a="211448404" X-IronPort-AV: E=Sophos;i="5.75,375,1589266800"; d="scan'208";a="211448404" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jul 2020 06:08:22 -0700 IronPort-SDR: 3akDlpX0NGv6JDVObZwOaoNfYAK+BL5J1IFZJcFkgPk5tXgl9LBxLBYOh9HHuLOQTGiTPWRlir OSv9v+A6WUfw== X-IronPort-AV: E=Sophos;i="5.75,375,1589266800"; d="scan'208";a="461684426" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.33.168]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 20 Jul 2020 06:08:20 -0700 Date: Mon, 20 Jul 2020 14:08:16 +0100 From: Bruce Richardson To: Ciara Power Cc: kevin.laatz@intel.com, thomas@monjalon.net, ferruh.yigit@intel.com, arybchenko@solarflare.com, dev@dpdk.org, keith.wiles@intel.com, Louise Kilheeney Message-ID: <20200720130816.GB726@bricha3-MOBL.ger.corp.intel.com> References: <20200612105344.15383-1-ciara.power@intel.com> <20200720111907.30717-1-ciara.power@intel.com> <20200720111907.30717-3-ciara.power@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200720111907.30717-3-ciara.power@intel.com> Subject: Re: [dpdk-dev] [PATCH v6 2/3] test/test_telemetry_data: add unit tests for data to JSON X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Jul 20, 2020 at 12:19:06PM +0100, Ciara Power wrote: > From: Louise Kilheeney > > 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 > Signed-off-by: Ciara Power > > --- > 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 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 > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#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 >