From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by dpdk.org (Postfix) with ESMTP id C12371B115 for ; Wed, 3 Oct 2018 21:06:57 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 4AF0B4007B for ; Wed, 3 Oct 2018 21:06:57 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 27E9B40080; Wed, 3 Oct 2018 21:06:57 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on bernadotte.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-0.6 required=5.0 tests=ALL_TRUSTED,AWL,URIBL_SBL, URIBL_SBL_A autolearn=disabled version=3.4.1 X-Spam-Score: -0.6 Received: from [192.168.1.59] (host-90-232-60-155.mobileonline.telia.com [90.232.60.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 4820D4007A; Wed, 3 Oct 2018 21:06:54 +0200 (CEST) To: Kevin Laatz , dev@dpdk.org Cc: harry.van.haaren@intel.com, stephen@networkplumber.org, gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, thomas@monjalon.net, Ciara Power , Brian Archbold References: <1535026093-101872-1-git-send-email-ciara.power@intel.com> <20181003173612.67101-1-kevin.laatz@intel.com> <20181003173612.67101-4-kevin.laatz@intel.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Message-ID: <714ed985-0ed5-ff3f-726d-0c86131c7903@lysator.liu.se> Date: Wed, 3 Oct 2018 21:06:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181003173612.67101-4-kevin.laatz@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Virus-Scanned: ClamAV using ClamSMTP Subject: Re: [dpdk-dev] [PATCH v2 03/10] telemetry: add client feature and sockets 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: Wed, 03 Oct 2018 19:06:58 -0000 On 2018-10-03 19:36, Kevin Laatz wrote: > From: Ciara Power > > This patch introduces clients to the telemetry API. > > When a client makes a connection through the initial telemetry > socket, they can send a message through the socket to be > parsed. Register messages are expected through this socket, to > enable clients to register and have a client socket setup for > future communications. > > A TAILQ is used to store all clients information. Using this, the > client sockets are polled for messages, which will later be parsed > and dealt with accordingly. > > Functionality that make use of the client sockets were introduced > in this patch also, such as writing to client sockets, and sending > error responses. > > Signed-off-by: Ciara Power > Signed-off-by: Brian Archbold > Signed-off-by: Kevin Laatz > --- > lib/librte_telemetry/meson.build | 2 + > lib/librte_telemetry/rte_telemetry.c | 365 +++++++++++++++++++++++++- > lib/librte_telemetry/rte_telemetry_internal.h | 25 ++ > mk/rte.app.mk | 2 +- > 4 files changed, 390 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_telemetry/meson.build b/lib/librte_telemetry/meson.build > index 7716076..0ccfa36 100644 > --- a/lib/librte_telemetry/meson.build > +++ b/lib/librte_telemetry/meson.build > @@ -5,3 +5,5 @@ sources = files('rte_telemetry.c') > headers = files('rte_telemetry.h', 'rte_telemetry_internal.h') > deps += ['metrics', 'ethdev'] > cflags += '-DALLOW_EXPERIMENTAL_API' > +jansson = cc.find_library('jansson', required: true) > +ext_deps += jansson > diff --git a/lib/librte_telemetry/rte_telemetry.c b/lib/librte_telemetry/rte_telemetry.c > index 0c99d66..7726fd4 100644 > --- a/lib/librte_telemetry/rte_telemetry.c > +++ b/lib/librte_telemetry/rte_telemetry.c > @@ -7,6 +7,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -16,6 +17,8 @@ > #include "rte_telemetry.h" > #include "rte_telemetry_internal.h" > > +#define BUF_SIZE 1024 > +#define ACTION_POST 1 > #define SLEEP_TIME 10 > > #define DEFAULT_DPDK_PATH "/var/run/.rte_telemetry" > @@ -34,6 +37,91 @@ rte_telemetry_is_port_active(int port_id) > > TELEMETRY_LOG_ERR("port_id: %d is invalid, not active", > port_id); > + > + return 0; > +} > + > +int32_t > +rte_telemetry_write_to_socket(struct telemetry_impl *telemetry, > + const char *json_string) > +{ > + int ret; > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Could not initialise TELEMETRY_API"); > + return -1; > + } 1.8.1 here again, and in many instances below. > + > + if (!telemetry->request_client) { > + TELEMETRY_LOG_ERR("No client has been chosen to write to"); > + return -1; > + } > + > + if (!json_string) { > + TELEMETRY_LOG_ERR("Invalid JSON string!"); > + return -1; > + } > + > + ret = send(telemetry->request_client->fd, > + json_string, strlen(json_string), 0); How would this code handle a partial success (as in, for example, half of the string fits the socket buffer)? In not, maybe switching over to a SOCK_SEQPACKET AF_UNIX socket would be the best way around it. > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Failed to write to socket for client: %s", > + telemetry->request_client->file_path); > + return -1; > + } > + > + return 0; > +} > + > +int32_t > +rte_telemetry_send_error_response(struct telemetry_impl *telemetry, > + int error_type) > +{ > + int ret; > + const char *status_code, *json_buffer; > + json_t *root; > + > + if (error_type == -EPERM) > + status_code = "Status Error: Unknown"; > + else if (error_type == -EINVAL) > + status_code = "Status Error: Invalid Argument 404"; > + else if (error_type == -ENOMEM) > + status_code = "Status Error: Memory Allocation Error"; > + else { > + TELEMETRY_LOG_ERR("Invalid error type"); > + return -EINVAL; > + } > + > + root = json_object(); > + > + if (!root) { > + TELEMETRY_LOG_ERR("Could not create root JSON object"); > + return -EPERM; > + } > + > + ret = json_object_set_new(root, "status_code", > + json_string(status_code)); > + > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Status code field cannot be set"); No json_decref()? > + return -EPERM; > + } > + > + ret = json_object_set_new(root, "data", json_null()); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Data field cannot be set"); ... and a json_decref() here too? > + return -EPERM; > + } > + > + json_buffer = json_dumps(root, JSON_INDENT(2)); > + json_decref(root); > + > + ret = rte_telemetry_write_to_socket(telemetry, json_buffer); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Could not write to socket"); > + return -EPERM; > + } > + > return 0; > } > > @@ -110,8 +198,7 @@ rte_telemetry_initial_accept(struct telemetry_impl *telemetry) > int pid; > > RTE_ETH_FOREACH_DEV(pid) { > - telemetry->reg_index = > - rte_telemetry_reg_ethdev_to_metrics(pid); > + telemetry->reg_index = rte_telemetry_reg_ethdev_to_metrics(pid); > break; > } > > @@ -126,6 +213,33 @@ rte_telemetry_initial_accept(struct telemetry_impl *telemetry) > } > > static int32_t > +rte_telemetry_read_client(struct telemetry_impl *telemetry) > +{ > + char buf[BUF_SIZE]; > + int ret, buffer_read = 0; > + errno = 0; Generally speaking, you don't touch errno unless your function fails. In this case, I don't see the point at all. > + > + buffer_read = read(telemetry->accept_fd, buf, BUF_SIZE-1); This and the below code seem to assume that read() returns one and only one message, but on a SOCK_STREAM, there is no such thing as a message. It's a byte stream, and you need to provide your own framing, or have an application protocol which allows only have one outstanding request. If you do the latter, you still need to allow for "short" (partial) read()s (i.e. re-read() until done). > + buf[buffer_read] = '\0'; If buffer_read == -1, the above isn't a good idea. > + > + if (buffer_read == -1) { > + TELEMETRY_LOG_ERR("Read error"); > + return -1; > + } else if (buffer_read == 0) { > + close(telemetry->accept_fd); > + telemetry->accept_fd = 0; > + } else { > + ret = rte_telemetry_parse_client_message(telemetry, buf); > + if (ret < 0) > + TELEMETRY_LOG_WARN("Parse message failed"); > + close(telemetry->accept_fd); > + telemetry->accept_fd = 0; Maybe put the cleanup actions behind a goto label? > + } > + > + return 0; > +} > + > +static int32_t > rte_telemetry_accept_new_client(struct telemetry_impl *telemetry) > { > int ret; > @@ -136,8 +250,8 @@ rte_telemetry_accept_new_client(struct telemetry_impl *telemetry) > TELEMETRY_LOG_ERR("Listening error with server fd"); > return -1; > } > - telemetry->accept_fd = accept(telemetry->server_fd, NULL, NULL); > > + telemetry->accept_fd = accept(telemetry->server_fd, NULL, NULL); > if (telemetry->accept_fd > 0 && > telemetry->metrics_register_done == 0) { > ret = rte_telemetry_initial_accept(telemetry); > @@ -146,6 +260,31 @@ rte_telemetry_accept_new_client(struct telemetry_impl *telemetry) > return -1; > } > } > + } else { > + ret = rte_telemetry_read_client(telemetry); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Failed to read socket buffer"); > + return -1; > + } > + } > + > + return 0; > +} > + > +static int32_t > +rte_telemetry_read_client_sockets(struct telemetry_impl *telemetry) > +{ > + telemetry_client *client; > + char client_buf[BUF_SIZE]; > + int bytes; > + > + TAILQ_FOREACH(client, &telemetry->client_list_head, client_list) { > + bytes = read(client->fd, client_buf, BUF_SIZE-1); > + client_buf[bytes] = '\0'; > + > + if (bytes > 0) { > + telemetry->request_client = client; > + } > } > > return 0; > @@ -168,6 +307,12 @@ rte_telemetry_run(void *userdata) > return -1; > } > > + ret = rte_telemetry_read_client_sockets(telemetry); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Client socket read failed"); > + return -1; > + } > + > return 0; > } > > @@ -268,6 +413,7 @@ rte_telemetry_init() > TELEMETRY_LOG_ERR("TELEMETRY cleanup failed"); > return -EPERM; > } > + TAILQ_INIT(&static_telemetry->client_list_head); > > pthread_attr_init(&attr); > ret = rte_ctrl_thread_create(&static_telemetry->thread_id, > @@ -285,11 +431,39 @@ rte_telemetry_init() > return 0; > } > > +static int32_t > +rte_telemetry_client_cleanup(struct telemetry_client *client) > +{ > + int ret; > + > + ret = close(client->fd); > + free(client->file_path); > + free(client); > + > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Close client socket failed"); > + return -EPERM; There are other close() calls for which the return code is not checked. The code should be consistent, one way or the other. > + } > + > + return 0; > +} > + > int32_t > rte_telemetry_cleanup(void) > { > int ret; > struct telemetry_impl *telemetry = static_telemetry; > + telemetry_client *client, *temp_client; > + > + TAILQ_FOREACH_SAFE(client, &telemetry->client_list_head, client_list, > + temp_client) { > + TAILQ_REMOVE(&telemetry->client_list_head, client, client_list); > + ret = rte_telemetry_client_cleanup(client); > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Client cleanup failed"); > + return -EPERM; > + } > + } > > ret = close(telemetry->server_fd); > if (ret < 0) { > @@ -306,6 +480,191 @@ rte_telemetry_cleanup(void) > return 0; > } > > +int32_t > +rte_telemetry_unregister_client(struct telemetry_impl *telemetry, > + const char *client_path) > +{ > + int ret; > + telemetry_client *client, *temp_client; > + > + if (!telemetry) { > + TELEMETRY_LOG_WARN("TELEMETRY is not initialised"); > + return -ENODEV; > + } > + > + if (!client_path) { > + TELEMETRY_LOG_ERR("Invalid client path"); > + goto einval_fail; > + } > + > + if (TAILQ_EMPTY(&telemetry->client_list_head)) { > + TELEMETRY_LOG_ERR("There are no clients currently registered"); > + return -EPERM; > + } > + > + TAILQ_FOREACH_SAFE(client, &telemetry->client_list_head, client_list, > + temp_client) { > + if (strcmp(client_path, client->file_path) == 0) { > + TAILQ_REMOVE(&telemetry->client_list_head, client, > + client_list); > + ret = rte_telemetry_client_cleanup(client); > + > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Client cleanup failed"); > + return -EPERM; > + } > + > + return 0; > + } > + } > + > + TELEMETRY_LOG_WARN("Couldn't find client, possibly not registered yet."); > + return -1; > + > +einval_fail: > + ret = rte_telemetry_send_error_response(telemetry, -EINVAL); > + if (ret < 0) > + TELEMETRY_LOG_ERR("Could not send error"); > + return -EINVAL; > +} > + > +int32_t > +rte_telemetry_register_client(struct telemetry_impl *telemetry, > + const char *client_path) > +{ > + int ret, fd; > + struct sockaddr_un addrs = {0}; Setting all the fields, so no need to clear the struct. > + > + if (!telemetry) { > + TELEMETRY_LOG_ERR("Could not initialize TELEMETRY API"); > + return -ENODEV; > + } > + > + if (!client_path) { > + TELEMETRY_LOG_ERR("Invalid client path"); > + return -EINVAL; > + } > + > + telemetry_client *client; > + TAILQ_FOREACH(client, &telemetry->client_list_head, client_list) { > + if (strcmp(client_path, client->file_path) == 0) { > + TELEMETRY_LOG_WARN("'%s' already registered", > + client_path); > + return -EINVAL; > + } > + } > + fd = socket(AF_UNIX, SOCK_STREAM, 0); > + if (fd == -1) { > + TELEMETRY_LOG_ERR("Client socket error"); > + return -EACCES; > + } > + ret = rte_telemetry_set_socket_nonblock(fd); You could also use SOCK_NONBLOCK at the time of the socket() call. > + if (ret < 0) { > + TELEMETRY_LOG_ERR("Could not set socket to NONBLOCK"); > + return -EPERM; > + } > + > + addrs.sun_family = AF_UNIX; > + strlcpy(addrs.sun_path, client_path, sizeof(addrs.sun_path)); > + telemetry_client *new_client = > + (telemetry_client *)malloc(sizeof(telemetry_client)); malloc() returns a void pointer, so no need to cast.