From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 3D4C65398 for ; Fri, 31 Aug 2018 01:58:01 +0200 (CEST) Received: by mail-wr1-f67.google.com with SMTP id j26-v6so9566986wre.2 for ; Thu, 30 Aug 2018 16:58:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=VItHyCPrpTEzG7s3sC4pibVy2qZhTYi2BsTZ6JRQbYI=; b=H4aCSWZb7+QTzv7A8T6iCihvutiTFFezpZ7rW0AVhp8CKDO7yDeNIw2Ec4n6J0ZlrH DwMdeFuMPm1mPXMfeczf9J5RWyo/oBLqOa/x3OW5uaDwAvYmmIm62EEsGzy+xIX591BT tGckcYkozuM0tWOeEcXukzU/fMWs5KJfx/7cl25XIbMLlcDkBb2261wDBnuX228pLdm1 k9p7FPDC5vGmq5Fta9wyV/wWMgZmOIYX9G/F54T/ks/eSo/m3M+WwYFw62Nf7IBmjvuN mIi2PTmGsckI4DUeCe+6KwiqnBTTUWm4Yp13bkD3oDV+pYKChSsmtmXngOVkkBi6+yT8 k8yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=VItHyCPrpTEzG7s3sC4pibVy2qZhTYi2BsTZ6JRQbYI=; b=hpj3XnJ/2WKaKzA08N6vw7YgDpqNvDG1y7u92KdOQK0oillUQLlvcH+OFfCpQ8XOSn /UrSsvZ4/2IFiRZ/8sIRTwbQpbNtcqIKMRsPzIeAgDz8rqGcNRmZiWPjK8NKbZBE5CQr jpLQiaZgxmFmwR0BiuugbGNmO4VyiwRjfOTEgsV1gmIxHt2X65gdLnVuRr0aUB0f6gvh xDaKK97dUvAriCPBZQBnSoysdf3CSpa2HxzQM8OutzXX60u/VtP8DNrtLp5YFkUS5yMF nEDwBEMtAib7TY8zdYRg5HE7Y1SZ3Obqm9CfdLK9qA9mU9agJFRy96or9eTTzOI6jS/x 9cLg== X-Gm-Message-State: APzg51BvCr6U6lowYtsDsBnIJbF1uyjtdsokdiScJu/Vk9vXCMozThYi Ki2/kpVjxl33gb8fC/mGH3uoVg== X-Google-Smtp-Source: ANB0VdZ4VN8GU2DxaRgWWJP8xCRxLO7AZJd1vAgwXLVIxsJblaRgCFWAeh0iHYLxqqS4al11GhzYyw== X-Received: by 2002:adf:c454:: with SMTP id a20-v6mr8908516wrg.20.1535673480662; Thu, 30 Aug 2018 16:58:00 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id u1-v6sm5968760wrt.59.2018.08.30.16.57.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 30 Aug 2018 16:57:59 -0700 (PDT) Date: Fri, 31 Aug 2018 01:57:43 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Ciara Power Cc: harry.van.haaren@intel.com, brian.archbold@intel.com, emma.kenny@intel.com, dev@dpdk.org Message-ID: <20180830235742.hmn2ssdkdjt76egi@bidouze.vm.6wind.com> References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <1535026093-101872-5-git-send-email-ciara.power@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1535026093-101872-5-git-send-email-ciara.power@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 04/11] telemetry: add parser for client socket messages 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: , X-List-Received-Date: Thu, 30 Aug 2018 23:58:01 -0000 Hi, On Thu, Aug 23, 2018 at 01:08:06PM +0100, Ciara Power wrote: > This patch adds the parser file. This is used to parse any > messages that are received on any of the client sockets. > > Currently, the unregister functionality works using the parser. > Functionality relating to getting statistic values for certain ports > will be added in a subsequent patch, however the parsing involved > for that command is added in this patch. > > Some of the parser code included is in preparation for future > functionality, that is not implemented yet in this patchset. > > Signed-off-by: Ciara Power > Signed-off-by: Brian Archbold > --- > lib/librte_telemetry/Makefile | 1 + > lib/librte_telemetry/meson.build | 4 +- > lib/librte_telemetry/rte_telemetry.c | 9 + > lib/librte_telemetry/rte_telemetry_internal.h | 3 + > lib/librte_telemetry/rte_telemetry_parser.c | 585 ++++++++++++++++++++++++++ > lib/librte_telemetry/rte_telemetry_parser.h | 13 + > 6 files changed, 613 insertions(+), 2 deletions(-) > create mode 100644 lib/librte_telemetry/rte_telemetry_parser.c > create mode 100644 lib/librte_telemetry/rte_telemetry_parser.h > > diff --git a/lib/librte_telemetry/Makefile b/lib/librte_telemetry/Makefile > index bda3788..df8fdd9 100644 > --- a/lib/librte_telemetry/Makefile > +++ b/lib/librte_telemetry/Makefile > @@ -19,6 +19,7 @@ LIBABIVER := 1 > > # library source files > SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) := rte_telemetry.c > +SRCS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += rte_telemetry_parser.c > > # export include files > SYMLINK-$(CONFIG_RTE_LIBRTE_TELEMETRY)-include := rte_telemetry.h > diff --git a/lib/librte_telemetry/meson.build b/lib/librte_telemetry/meson.build > index 0ccfa36..7450f96 100644 > --- a/lib/librte_telemetry/meson.build > +++ b/lib/librte_telemetry/meson.build > @@ -1,8 +1,8 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2018 Intel Corporation > > -sources = files('rte_telemetry.c') > -headers = files('rte_telemetry.h', 'rte_telemetry_internal.h') > +sources = files('rte_telemetry.c', 'rte_telemetry_parser.c') > +headers = files('rte_telemetry.h', 'rte_telemetry_internal.h', 'rte_telemetry_parser.h') > deps += ['metrics', 'ethdev'] > cflags += '-DALLOW_EXPERIMENTAL_API' > jansson = cc.find_library('jansson', required: true) > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > index e9dd022..c6c6612 100644 > --- a/lib/librte_telemetry/rte_telemetry.c > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -15,6 +15,7 @@ > > #include "rte_telemetry.h" > #include "rte_telemetry_internal.h" > +#include "rte_telemetry_parser.h" > > #define BUF_SIZE 1024 > #define ACTION_POST 1 > @@ -272,6 +273,8 @@ rte_telemetry_accept_new_client(struct telemetry_impl *telemetry) > static int32_t > rte_telemetry_read_client_sockets(struct telemetry_impl *telemetry) > { > + int ret; > + > telemetry_client *client; > TAILQ_FOREACH(client, &telemetry->client_list_head, client_list) { > char client_buf[BUF_SIZE]; > @@ -279,6 +282,12 @@ rte_telemetry_read_client_sockets(struct telemetry_impl *telemetry) > client_buf[bytes] = '\0'; > if (bytes > 0) { > telemetry->request_client = client; > + ret = rte_telemetry_parse(telemetry, client_buf); > + if (ret < 0) { > + TELEMETRY_LOG_WARN("Warning - Parse socket " > + "input failed: %i\n", ret); I see LOG_WARN being always preceded by "Warning - ", LOG_ERR by "Error - ", and so on. Wouldn't it be simpler to have the prefix inserted systematically? I have seen at least one mistake on a LOG_ERR message (in another patch). Also Shreyansh already remarked about it, but you may have doubled the newline. > + return -1; > + } > } > } > return 0; > diff --git a/lib/librte_telemetry/rte_telemetry_internal.h b/lib/librte_telemetry/rte_telemetry_internal.h > index e3292cf..b057794 100644 > --- a/lib/librte_telemetry/rte_telemetry_internal.h > +++ b/lib/librte_telemetry/rte_telemetry_internal.h > @@ -58,4 +58,7 @@ int32_t > rte_telemetry_unregister_client(struct telemetry_impl *telemetry, > const char *client_path); > > +int32_t > +rte_telemetry_check_port_activity(int port_id); > + > #endif > diff --git a/lib/librte_telemetry/rte_telemetry_parser.c b/lib/librte_telemetry/rte_telemetry_parser.c > new file mode 100644 > index 0000000..571c991 > --- /dev/null > +++ b/lib/librte_telemetry/rte_telemetry_parser.c > @@ -0,0 +1,585 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include "rte_telemetry_internal.h" > + > +#define ACTION_GET 0 > +#define ACTION_DELETE 2 An enum would be cleaner here, I don't see a reason for these values to be defines. > + > +struct command { struct rte_telemetry_command might be a better name. > + char *command_text; command.command_text? Why not simply text? > + int (*comm_func_ptr)(struct telemetry_impl *, int, json_t *); Function pointers should be typedef for readability. > +} command; > + > +static int32_t > +rte_telemetry_command_clients(struct telemetry_impl *telemetry, int action, > + json_t *data) > +{ > + int ret; blank line missing here. However, it seems ret is never actually used. All checks could be performed against the function calls directly, as the return value is never used itself, -1 is always returned. > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + if (action != ACTION_DELETE) { > + TELEMETRY_LOG_WARN("Warning - Invalid action for this " > + "command\n"); > + goto einval_fail; > + } > + > + if (!json_is_object(data)) { > + TELEMETRY_LOG_WARN("Warning - Invalid data provided for this " > + "command\n"); > + goto einval_fail; > + } > + > + json_t *client_path = json_object_get(data, "client_path"); Coding style guide dictates local variables to be defined at start of scope. [※]: https://doc.dpdk.org/guides/contributing/coding_style.html#local-variables > + if (!json_is_string(client_path)) { > + TELEMETRY_LOG_WARN("Warning - Command value is not a string\n"); > + goto einval_fail; > + } > + > + const char *client_path_string = json_string_value(client_path); This variable is not used afterward, why not write: if (rte_telemetry_unregister_client(telemetry, json_string_value(client_path))) { TELEMETRY_LOG_ERR("Could not unregister client"); goto einval_fail; } > + > + ret = rte_telemetry_unregister_client(telemetry, client_path_string); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Error - could not unregister client\n"); > + goto einval_fail; > + } > + > + return 0; > + > +einval_fail: > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > +} > + > +static int32_t > +rte_telemetry_command_ports(struct telemetry_impl *telemetry, int action, > + json_t *data) > +{ > + int ret; > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + if (!json_is_null(data)) { > + TELEMETRY_LOG_WARN("Warning - Data should be NULL JSON object " > + "for 'ports' command\n"); > + goto einval_fail; > + } > + > + if (action != ACTION_GET) { > + TELEMETRY_LOG_WARN("Warning - Invalid action for this " > + "command\n"); > + goto einval_fail; > + } > + > + return 0; > + > +einval_fail: > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > +} > + > +static int32_t > +rte_telemetry_command_ports_details(struct telemetry_impl *telemetry, > + int action, json_t *data) > +{ > + int ret; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + if (action != ACTION_GET) { > + TELEMETRY_LOG_WARN("Warning - Invalid action for this " > + "command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; Previous functions used gotos for dealing with errors, this one repeats the pattern. Seems like an oversight. > + } > + > + if (!json_is_object(data)) { > + TELEMETRY_LOG_WARN("Warning - Invalid data provided for this " > + "command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + json_t *port_ids_json = json_object_get(data, "ports"); > + if (!json_is_array(port_ids_json)) { > + TELEMETRY_LOG_WARN("Warning - Invalid Port ID array\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + uint64_t num_port_ids = json_array_size(port_ids_json); > + int port_ids[num_port_ids]; > + RTE_SET_USED(port_ids); > + size_t index; > + json_t *value; Declare those at start of scope. > + > + json_array_foreach(port_ids_json, index, value) { > + if (!json_is_integer(value)) { > + TELEMETRY_LOG_WARN("Warning - Port ID given is " > + "invalid\n"); > + ret = rte_telemetry_send_error_response(telemetry, > + -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send " > + "error\n"); > + return -1; > + } > + port_ids[index] = json_integer_value(value); > + } > + > + return 0; > +} > + Some previous errors are repeated afterward but I won't point each of them. [snip] > + > +static int32_t > +rte_telemetry_stat_names_to_ids(struct telemetry_impl *telemetry, > + const char * const *stat_names, uint32_t *stat_ids, > + uint64_t num_stat_names) > +{ > + struct rte_metric_name *names; > + int ret; > + > + if (stat_names == NULL) { > + TELEMETRY_LOG_WARN("Warning - Invalid stat_names argument\n"); > + goto einval_fail; > + } > + > + if (num_stat_names <= 0) { > + TELEMETRY_LOG_WARN("Warning - Invalid num_stat_names " > + "argument\n"); > + goto einval_fail; > + } > + > + int num_metrics = rte_metrics_get_names(NULL, 0); > + if (num_metrics < 0) { > + TELEMETRY_LOG_ERR("Error - Cannot get metrics count\n"); > + goto eperm_fail; > + } else if (num_metrics == 0) { > + TELEMETRY_LOG_WARN("Warning - No metrics have been " > + "registered\n"); > + goto eperm_fail; > + } > + > + names = malloc(sizeof(struct rte_metric_name) * num_metrics); > + if (names == NULL) { > + TELEMETRY_LOG_ERR("Error - Cannot allocate memory for names\n"); > + ret = rte_telemetry_send_error_response(telemetry, -ENOMEM); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + ret = rte_metrics_get_names(names, num_metrics); > + if (ret < 0 || ret > num_metrics) { > + TELEMETRY_LOG_ERR("Error - Cannot get metrics names\n"); > + free(names); > + goto eperm_fail; > + } > + > + uint32_t i, k; > + k = 0; > + for (i = 0; i < (uint32_t)num_stat_names; i++) { > + uint32_t j; > + for (j = 0; j < (uint32_t)num_metrics; j++) { > + if (strcmp(stat_names[i], names[j].name) == 0) { > + stat_ids[k] = j; > + k++; > + break; > + } > + } > + } > + > + if (k != num_stat_names) { > + TELEMETRY_LOG_WARN("Warning - Invalid stat names provided\n"); It would be better to provide the user with a list of requested names that are invalid. > + free(names); > + goto einval_fail; > + } > + You are not freeing "names" here, nor are you returning it. > + return 0; > + > +einval_fail: > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + > +eperm_fail: > + ret = rte_telemetry_send_error_response(telemetry, -EPERM); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > +} > + > +int32_t > +rte_telemetry_command_ports_all_stat_values(struct telemetry_impl *telemetry, > + int action, json_t *data) > +{ > + int ret, num_metrics; > + struct rte_metric_name *names; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + if (action != ACTION_GET) { > + TELEMETRY_LOG_WARN("Warning - Invalid action for this command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + if (json_is_object(data)) { > + TELEMETRY_LOG_WARN("Warning - Invalid data provided for this command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + num_metrics = rte_metrics_get_names(NULL, 0); > + if (num_metrics < 0) { > + TELEMETRY_LOG_ERR("Error - Cannot get metrics count\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } else if (num_metrics == 0) { > + TELEMETRY_LOG_ERR("Error - No metrics to display (none have been registered)\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EPERM); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + names = malloc(sizeof(struct rte_metric_name) * num_metrics); > + if (!names) { > + TELEMETRY_LOG_ERR("Error - Cannot allocate memory\n"); > + ret = rte_telemetry_send_error_response(telemetry, > + -ENOMEM); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + uint64_t num_port_ids = 0; > + const char *stat_names[num_metrics]; > + uint32_t stat_ids[num_metrics]; > + int p; > + > + RTE_ETH_FOREACH_DEV(p) { > + num_port_ids++; > + } > + if (!num_port_ids) { > + TELEMETRY_LOG_WARN("Warning - No active ports\n"); > + ret = rte_telemetry_send_error_response(telemetry, > + -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + goto fail; > + } > + > + ret = rte_metrics_get_names(names, num_metrics); > + int i; > + for (i = 0; i < num_metrics; i++) > + stat_names[i] = names[i].name; > + > + ret = rte_telemetry_stat_names_to_ids(telemetry, stat_names, stat_ids, > + num_metrics); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Error - Could not convert stat names to IDs\n"); > + goto fail; > + } > + return 0; The retrieved IDs here are not used, is it because this function is extended in another patch? You also already use rte_metrics_get_names() to generate a request, that will itself do the same, compare the two and assign IDs according to their index in the list. This could probably be written in a simpler and more concise way. This is rather awkward to divide the patches this way. It is harder right now to judge this function and how stat_names_to_ids is implemented, because the finality of it is not yet available. It was not necessary to add this command alongside the others, it could come later. > + > +fail: > + free(names); > + return -1; > +} > + > +int32_t > +rte_telemetry_command_ports_stats_values_by_name(struct telemetry_impl > + *telemetry, int action, json_t *data) > +{ > + int ret; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + if (action != ACTION_GET) { > + TELEMETRY_LOG_WARN("Warning - Invalid action for this " > + "command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + if (!json_is_object(data)) { > + TELEMETRY_LOG_WARN("Warning - Invalid data provided for this " > + "command\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + json_t *port_ids_json = json_object_get(data, "ports"); > + json_t *stat_names_json = json_object_get(data, "stats"); > + if (!json_is_array(port_ids_json) || > + !json_is_array(stat_names_json)) { > + TELEMETRY_LOG_WARN("Warning - Invalid input data array(s)\n"); > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send error\n"); > + return -1; > + } > + > + uint64_t num_port_ids = json_array_size(port_ids_json); > + uint32_t port_ids[num_port_ids]; > + size_t index; > + json_t *value; > + > + json_array_foreach(port_ids_json, index, value) { > + if (!json_is_integer(value)) { > + TELEMETRY_LOG_WARN("Warning - Port ID given is not " > + "valid\n"); > + ret = rte_telemetry_send_error_response(telemetry, > + -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send " > + "error\n"); > + return -1; > + } > + port_ids[index] = json_integer_value(value); > + ret = rte_telemetry_check_port_activity(port_ids[index]); > + if (ret < 1) { > + ret = rte_telemetry_send_error_response(telemetry, > + -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send " > + "error\n"); > + return -1; > + } > + } > + > + uint64_t num_stat_names = json_array_size(stat_names_json); > + const char *stat_names[num_stat_names]; > + > + json_array_foreach(stat_names_json, index, value) { > + if (!json_is_string(value)) { > + TELEMETRY_LOG_WARN("Warning - Stat Name given is not a " > + "string\n"); > + ret = rte_telemetry_send_error_response(telemetry, > + -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Error - Could not send " > + "error\n"); > + return -1; > + } > + stat_names[index] = json_string_value(value); > + } > + > + uint32_t stat_ids[num_stat_names]; > + ret = rte_telemetry_stat_names_to_ids(telemetry, stat_names, stat_ids, > + num_stat_names); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Error - Could not convert stat names to " > + "IDs\n"); > + return -1; > + } > + return 0; > +} > + > +static int32_t > +rte_telemetry_parse_command(struct telemetry_impl *telemetry, int action, > + const char *command, json_t *data) > +{ > + int ret; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Error - Invalid telemetry argument\n"); > + return -1; > + } > + > + struct command commands[] = { > + {.command_text = "clients", > + .comm_func_ptr = &rte_telemetry_command_clients}, > + {.command_text = "ports", > + .comm_func_ptr = &rte_telemetry_command_ports}, > + {.command_text = "ports_details", > + .comm_func_ptr = &rte_telemetry_command_ports_details}, > + {.command_text = "port_stats", > + .comm_func_ptr = &rte_telemetry_command_port_stats}, > + {.command_text = "ports_stats_values_by_name", > + .comm_func_ptr = > + &rte_telemetry_command_ports_stats_values_by_name}, > + {.command_text = "ports_all_stat_values", > + .comm_func_ptr = > + &rte_telemetry_command_ports_all_stat_values} These command names are unnecessarily verbose, while still not saying exactly what the commands are. "clients" only supports "DELETE", but the function name does not reflect that. "ports" only supports "GET", but this is not explicit. And on the other hand, "rte_telemetry_command_ports_stats_values_by_name" is just too much. The coding style is wrong as well. With the proper definitions, something like this could be written: struct rte_telemetry_command commands[] = { { .text = "client", .fn = { [DELETE] = rte_tlm_client_unregister, }, }, { .text = "port", .fn = { [GET] = rte_tlm_port_get, }, }, ... }; > + }; > + > + const uint32_t num_commands = sizeof(commands)/sizeof(struct command); RTE_DIM() should be used for this. > + uint32_t i; > + Here action should be checked against the enum previously described, verifying that it is within range. > + for (i = 0; i < num_commands; i++) { > + if (strcmp(command, commands[i].command_text) == 0) { > + int ret = commands[i].comm_func_ptr(telemetry, action, > + data); With the above format, this would become if (strcmp(command, commands[i].text) == 0) { rte_telemetry_command_cb fn; fn = commands[i].fn[action]; if (fn == NULL) { /* Error invalid command */ return -1; } else if (fn(telemetry, data)) { /* Error log would already be provided * by the function itself. */ return -1; } return 0; } Regards, -- Gaëtan Rivet 6WIND