Skip to content

Commit 5d4c0e1

Browse files
xiazhveraxiazhvera
committed
update code review
1 parent daea57e commit 5d4c0e1

4 files changed

Lines changed: 34 additions & 62 deletions

File tree

‎include/aws/mqtt/private/mqtt_iot_sdk_metrics.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ AWS_MQTT_API size_t aws_mqtt_iot_sdk_metrics_compute_storage_size(const struct a
2828
AWS_MQTT_API void aws_mqtt_iot_sdk_metrics_storage_destroy(struct aws_mqtt_iot_sdk_metrics_storage *metrics_storage);
2929

3030
/**
31-
* Appends SDK metrics to the username field 1Code has comments. Press enter to view.
31+
* Appends SDK metrics to the username
3232
*
3333
* @param original_username The original username
3434
* @param metrics The metrics configuration

‎source/client.c‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -630,9 +630,6 @@ static void s_mqtt_client_init(
630630
struct aws_byte_cursor username_cur;
631631
if (connection->username) {
632632
username_cur = aws_byte_cursor_from_string(connection->username);
633-
} else {
634-
/* Create empty username cursor when username is null but metrics is set */
635-
username_cur = aws_byte_cursor_from_c_str("");
636633
}
637634

638635
/* Apply metrics to username if configured */
@@ -3387,7 +3384,7 @@ static int s_aws_mqtt_client_connection_311_set_metrics(void *impl, const struct
33873384
}
33883385

33893386
if (metrics != NULL && aws_mqtt_validate_iot_sdk_metrics_utf8(metrics) == AWS_OP_ERR) {
3390-
AWS_LOGF_DEBUG(
3387+
AWS_LOGF_ERROR(
33913388
AWS_LS_MQTT_CLIENT, "id=%p: Invalid utf8 or forbidden codepoints in metrics.", (void *)connection);
33923389
return aws_raise_error(AWS_ERROR_INVALID_UTF8);
33933390
}
@@ -3405,6 +3402,17 @@ static int s_aws_mqtt_client_connection_311_set_metrics(void *impl, const struct
34053402
if (metrics) {
34063403
/* Initialize metrics storage */
34073404
connection->metrics_storage = aws_mqtt_iot_sdk_metrics_storage_new(connection->allocator, metrics);
3405+
3406+
if (!connection->metrics_storage) {
3407+
int error = aws_last_error();
3408+
AWS_LOGF_ERROR(
3409+
AWS_LS_MQTT_CLIENT,
3410+
"id=%p: Failed to create IoT SDK metrics storage, error %d (%s)",
3411+
(void *)connection,
3412+
error,
3413+
aws_error_name(error));
3414+
return aws_raise_error(error);
3415+
}
34083416
}
34093417

34103418
return AWS_OP_SUCCESS;

‎source/mqtt_iot_sdk_metrics.c‎

Lines changed: 20 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111

1212
#include <stdio.h>
1313

14-
// MQTT payload size https://docs.aws.amazon.com/general/latest/gr/iot-core.html#thing-limits
15-
const size_t AWS_IOT_MAX_USERNAME_SIZE = 128 * 1024;
14+
// Use packet encoding limit for now: https://github.com/awslabs/aws-c-mqtt/blob/v0.13.3/source/packets.c#L26
15+
const size_t AWS_IOT_MAX_USERNAME_SIZE = UINT16_MAX;
1616
const size_t DEFAULT_QUERY_PARAM_COUNT = 10;
1717

1818
// Build username query string from params_list, the caller is responsible to init and clean up output_username
@@ -80,15 +80,20 @@ int aws_mqtt_append_sdk_metrics_to_username(
8080
if (!allocator) {
8181
return aws_raise_error(AWS_ERROR_INVALID_ARGUMENT);
8282
}
83+
struct aws_byte_cursor *local_original_username = NULL;
84+
if (original_username == NULL) {
85+
local_original_username = aws_byte_cursor_from_c_str("");
86+
} else {
87+
local_original_username = original_username;
88+
}
8389

8490
if (!metrics) {
8591
if (out_full_username_size) {
86-
*out_full_username_size = original_username->len;
92+
*out_full_username_size = local_original_username->len;
8793
}
8894

89-
if (output_username &&
90-
aws_byte_buf_init(output_username, allocator, original_username->len) == AWS_OP_SUCCESS) {
91-
aws_byte_buf_write(output_username, original_username->ptr, original_username->len);
95+
if (output_username) {
96+
return aws_byte_buf_init_copy_from_cursor(output_username, allocator, local_original_username->len);
9297
}
9398

9499
return AWS_OP_SUCCESS;
@@ -114,17 +119,18 @@ int aws_mqtt_append_sdk_metrics_to_username(
114119
struct aws_array_list params_list;
115120
aws_array_list_init_dynamic(&params_list, allocator, DEFAULT_QUERY_PARAM_COUNT, sizeof(struct aws_uri_param));
116121

117-
if (original_username && original_username->len > 0) {
122+
if (local_original_username && local_original_username->len > 0) {
118123
struct aws_byte_cursor question_mark_find;
119124

120-
if (AWS_OP_SUCCESS == aws_byte_cursor_find_exact(original_username, &question_mark_str, &question_mark_find)) {
121-
base_username_length = question_mark_find.ptr - original_username->ptr;
125+
if (AWS_OP_SUCCESS ==
126+
aws_byte_cursor_find_exact(local_original_username, &question_mark_str, &question_mark_find)) {
127+
base_username_length = question_mark_find.ptr - local_original_username->ptr;
122128
// Advance cursor to skip the "?" character
123129
aws_byte_cursor_advance(&question_mark_find, 1);
124130
aws_byte_buf_append(&metrics_string, &question_mark_find);
125131
aws_query_string_params(question_mark_find, &params_list);
126132
} else {
127-
base_username_length = original_username->len;
133+
base_username_length = local_original_username->len;
128134
}
129135
}
130136

@@ -163,7 +169,7 @@ int aws_mqtt_append_sdk_metrics_to_username(
163169
// Rebuild metrics string from params_list
164170
// First path to calculate total size
165171
size_t total_size = 0;
166-
s_build_username_query(original_username, base_username_length, &params_list, NULL, &total_size);
172+
s_build_username_query(local_original_username, base_username_length, &params_list, NULL, &total_size);
167173

168174
if (total_size > AWS_IOT_MAX_USERNAME_SIZE) {
169175
goto cleanup;
@@ -175,11 +181,10 @@ int aws_mqtt_append_sdk_metrics_to_username(
175181

176182
// build final output username
177183
if (s_build_username_query(
178-
original_username, base_username_length, &params_list, output_username, out_full_username_size)) {
184+
local_original_username, base_username_length, &params_list, output_username, out_full_username_size)) {
179185
goto cleanup;
180186
}
181187

182-
aws_byte_buf_clean_up(&metrics_string);
183188
result = AWS_OP_SUCCESS;
184189

185190
cleanup:
@@ -237,35 +242,6 @@ struct aws_mqtt_iot_sdk_metrics_storage *aws_mqtt_iot_sdk_metrics_storage_new(
237242

238243
struct aws_mqtt_iot_sdk_metrics *storage_view = &metrics_storage->storage_view;
239244

240-
// TODO Future Work: add metadata entries once we implemented the metadata feature
241-
//
242-
// if (aws_array_list_init_dynamic(
243-
// &metrics_storage->metadata_entries,
244-
// allocator,
245-
// metrics_options->metadata_count,
246-
// sizeof(struct aws_mqtt_metadata_entry))) {
247-
// goto metrics_storage_error;
248-
// }
249-
250-
// for (size_t i = 0; i < metrics_options->metadata_count; ++i) {
251-
// struct aws_mqtt_metadata_entry entry = metrics_options->metadata_entries[i];
252-
253-
// if (aws_byte_buf_append_and_update(&metrics_storage->storage, &entry.key)) {
254-
// goto metrics_storage_error;
255-
// }
256-
257-
// if (aws_byte_buf_append_and_update(&metrics_storage->storage, &entry.value)) {
258-
// goto metrics_storage_error;
259-
// }
260-
261-
// if (aws_array_list_push_back(&metrics_storage->metadata_entries, &entry)) {
262-
// goto metrics_storage_error;
263-
// }
264-
// }
265-
266-
// storage_view->metadata_entries = metrics_storage->metadata_entries.data;
267-
// storage_view->metadata_count = aws_array_list_length(&metrics_storage->metadata_entries);
268-
269245
if (metrics_options->library_name.len > 0) {
270246
metrics_storage->library_name = metrics_options->library_name;
271247
if (aws_byte_buf_append_and_update(&metrics_storage->storage, &metrics_storage->library_name)) {
@@ -285,9 +261,8 @@ struct aws_mqtt_iot_sdk_metrics_storage *aws_mqtt_iot_sdk_metrics_storage_new(
285261
if (aws_byte_buf_is_valid(&metrics_storage->storage)) {
286262
aws_byte_buf_clean_up(&metrics_storage->storage);
287263
}
288-
if (metrics_options != NULL) {
289-
aws_mem_release(allocator, metrics_storage);
290-
}
264+
265+
aws_mem_release(allocator, metrics_storage);
291266

292267
return NULL;
293268
}
@@ -314,13 +289,5 @@ int aws_mqtt_validate_iot_sdk_metrics_utf8(const struct aws_mqtt_iot_sdk_metrics
314289
return AWS_OP_ERR;
315290
}
316291

317-
// TODO: add metadata entries when enabled
318-
// for (size_t i = 0; i < metrics->metadata_count; ++i) {
319-
// if (aws_mqtt_validate_utf8_text(metrics->metadata_entries[i].key) ||
320-
// aws_mqtt_validate_utf8_text(metrics->metadata_entries[i].value)) {
321-
// return AWS_OP_ERR;
322-
// }
323-
// }
324-
325292
return AWS_OP_SUCCESS;
326293
}

‎tests/shared_utils_tests.c‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,7 @@ static int s_test_mqtt_append_sdk_metrics_empty(struct aws_allocator *allocator,
167167
struct aws_byte_buf output_username;
168168
AWS_ZERO_STRUCT(output_username);
169169

170-
struct aws_byte_cursor original_username = aws_byte_cursor_from_c_str("");
171-
172-
ASSERT_SUCCESS(
173-
aws_mqtt_append_sdk_metrics_to_username(allocator, &original_username, &metrics, &output_username, NULL));
170+
ASSERT_SUCCESS(aws_mqtt_append_sdk_metrics_to_username(allocator, NULL, &metrics, &output_username, NULL));
174171

175172
struct aws_byte_cursor output_cursor = aws_byte_cursor_from_buf(&output_username);
176173

0 commit comments

Comments
 (0)