From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <kevin.laatz@intel.com>
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
by dpdk.org (Postfix) with ESMTP id B480A1B3A5
for <dev@dpdk.org>; Tue, 23 Oct 2018 10:43:03 +0200 (CEST)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
23 Oct 2018 01:43:02 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.54,415,1534834800"; d="scan'208";a="243582217"
Received: from klaatz-mobl.ger.corp.intel.com (HELO [10.237.220.107])
([10.237.220.107])
by orsmga004.jf.intel.com with ESMTP; 23 Oct 2018 01:43:00 -0700
To: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= <mattias.ronnblom@ericsson.com>,
dev@dpdk.org
Cc: harry.van.haaren@intel.com, stephen@networkplumber.org,
gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, thomas@monjalon.net,
bruce.richardson@intel.com, Ciara Power <ciara.power@intel.com>,
Brian Archbold <brian.archbold@intel.com>
References: <20181016155802.2067-1-kevin.laatz@intel.com>
<20181022110014.82153-1-kevin.laatz@intel.com>
<20181022110014.82153-6-kevin.laatz@intel.com>
<b448b9fc-fdff-ccba-a0d6-608de070389f@ericsson.com>
From: "Laatz, Kevin" <kevin.laatz@intel.com>
Message-ID: <bdb0c989-70bc-a740-728d-ce57134400ca@intel.com>
Date: Tue, 23 Oct 2018 09:42:58 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <b448b9fc-fdff-ccba-a0d6-608de070389f@ericsson.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Subject: Re: [dpdk-dev] [PATCH v6 05/13] 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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Tue, 23 Oct 2018 08:43:04 -0000
Thanks for the review Mattias.
Will address the comments both here and in the 4/13 review.
Best regards,
Kevin
On 22/10/2018 15:05, Mattias Rönnblom wrote:
> On 2018-10-22 13:00, Kevin Laatz wrote:
>>
>> @@ -131,6 +217,38 @@ 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;
>
> No need to set it to zero.
>
>> +
>> + buffer_read = read(telemetry->accept_fd, buf, BUF_SIZE-1);
>> +
>> + if (buffer_read == -1) {
>> + TELEMETRY_LOG_ERR("Read error");
>> + return -1;
>> + } else if (buffer_read == 0) {
>> + goto close_socket;
>> + } else {
>
> I would have moved the 'ret' variable to this scope.
>
>>
>> +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';
>
> read() might return -1, and you'll be writing out-of-bounds.
>
>>
>> +int32_t
>> +rte_telemetry_parse_client_message(struct telemetry_impl *telemetry,
>> char *buf)
>> +{
>> + int ret, action_int;
>> + json_error_t error;
>> + json_t *root = json_loads(buf, 0, &error);
>> +
>> + if (root == NULL) {
>> + TELEMETRY_LOG_WARN("Could not load JSON object from data
>> passed in : %s",
>> + error.text);
>> + goto fail;
>> + } else if (!json_is_object(root)) {
>> + TELEMETRY_LOG_WARN("JSON Request is not a JSON object");
>> + json_decref(root);
>> + goto fail;
>> + }
>> +
>> + json_t *action = json_object_get(root, "action");
>> + if (action == NULL) {
>> + TELEMETRY_LOG_WARN("Request does not have action field");
>> + goto fail;
>> + } else if (!json_is_integer(action)) {
>> + TELEMETRY_LOG_WARN("Action value is not an integer");
>> + goto fail;
>> + }
>> +
>> + json_t *command = json_object_get(root, "command");
>> + if (command == NULL) {
>> + TELEMETRY_LOG_WARN("Request does not have command field");
>> + goto fail;
>> + } else if (!json_is_string(command)) {
>> + TELEMETRY_LOG_WARN("Command value is not a string");
>> + goto fail;
>> + }
>> +
>> + action_int = json_integer_value(action);
>> + if (action_int != ACTION_POST) {
>> + TELEMETRY_LOG_WARN("Invalid action code");
>> + goto fail;
>> + }
>> +
>> + if (strcmp(json_string_value(command), "clients") != 0) {
>> + TELEMETRY_LOG_WARN("Invalid command");
>> + goto fail;
>> + }
>> +
>> + json_t *data = json_object_get(root, "data");
>> + if (data == NULL) {
>> + TELEMETRY_LOG_WARN("Request does not have data field");
>> + goto fail;
>> + }
>> +
>> + json_t *client_path = json_object_get(data, "client_path");
>> + if (client_path == NULL) {
>> + TELEMETRY_LOG_WARN("Request does not have client_path field");
>> + goto fail;
>> + }
>> +
>> + if (!json_is_string(client_path)) {
>> + TELEMETRY_LOG_WARN("Client_path value is not a string");
>> + goto fail;
>> + }
>> +
>> + ret = rte_telemetry_register_client(telemetry,
>> + json_string_value(client_path));
>> + if (ret < 0) {
>> + TELEMETRY_LOG_ERR("Could not register client");
>> + telemetry->register_fail_count++;
>> + goto fail;
>> + }
>> +
>
> You're leaking the root object here, but maybe that's fixed in
> subsequent patches.
>
>> + return 0;
>> +
>> +fail:
>> + TELEMETRY_LOG_WARN("Client attempted to register with invalid
>> message");
>> + return -1;
>> +}
>> +