[dpdk-dev] [PATCH v6 05/13] telemetry: add client feature and sockets
Laatz, Kevin
kevin.laatz at intel.com
Tue Oct 23 10:42:58 CEST 2018
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;
>> +}
>> +
More information about the dev
mailing list