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