Re: [dpdk-dev] [PATCH v6 05/13] telemetry: add client feature and sockets

["Laatz, Kevin" <kevin.laatz@intel.com>]

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;
>> +}
>> +