AI review comments

TingDaoK · TingDaoK · commit 46159337f6e3 · 2025-07-02T17:03:18.000-07:00
diff --git a/include/aws/s3/private/s3_checksum_context.h b/include/aws/s3/private/s3_checksum_context.h
@@ -3,7 +3,7 @@
 
 /**
  * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
- * SPDX-License-Identifier: Apache-2.0.
+ * SPDX-License-Identifier: Apache-2.0
  */
 
 #include "aws/s3/s3_client.h"
@@ -43,19 +43,29 @@ struct aws_s3_upload_request_checksum_context {
  *
  * @param allocator Memory allocator
  * @param checksum_config Meta request level checksum configuration (can be NULL)
- * @param checksum_buffer User-provided checksum buffer (can be NULL)
  * @return New checksum context or NULL on error
  */
 AWS_S3_API
 struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_context_new(
     struct aws_allocator *allocator,
     const struct aws_s3_meta_request_checksum_config_storage *checksum_config);
 
+/**
+ * Create a new upload request checksum context with an existing checksum value.
+ * This is useful when resuming uploads or when the checksum is pre-calculated.
+ * Returns with reference count of 1.
+ *
+ * @param allocator Memory allocator
+ * @param checksum_config Meta request level checksum configuration (can be NULL)
+ * @param existing_checksum Pre-calculated checksum value as a byte cursor
+ * @return New checksum context or NULL on error (e.g., if checksum size doesn't match algorithm)
+ */
 AWS_S3_API
-struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_context_new_with_exist_checksum(
+struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_context_new_with_existing_checksum(
     struct aws_allocator *allocator,
     const struct aws_s3_meta_request_checksum_config_storage *checksum_config,
-    struct aws_byte_cursor exist_checksum);
+    struct aws_byte_cursor existing_checksum);
+
 /**
  * Acquire a reference to the upload request checksum context.
  * Use this when transferring ownership to another function/structure.
@@ -79,43 +89,55 @@ struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_co
     struct aws_s3_upload_request_checksum_context *context);
 
 /**
- * Check if checksum calculation is needed based on context state
+ * Check if checksum calculation is needed based on context state.
+ * Returns true if the context has a valid algorithm and the checksum has not been calculated yet.
+ *
+ * @param context The checksum context to check
+ * @return true if checksum calculation is needed, false otherwise
  */
 AWS_S3_API
 bool aws_s3_upload_request_checksum_context_should_calculate(
     const struct aws_s3_upload_request_checksum_context *context);
 
 /**
- * Check if checksum should be added to HTTP headers
+ * Check if checksum should be added to HTTP headers.
+ * Returns true if the context has a valid algorithm and the location is set to header.
+ *
+ * @param context The checksum context to check
+ * @return true if checksum should be added to headers, false otherwise
  */
 AWS_S3_API
 bool aws_s3_upload_request_checksum_context_should_add_header(
     const struct aws_s3_upload_request_checksum_context *context);
 
 /**
- * Check if checksum should be added as trailer (chunked encoding)
+ * Check if checksum should be added as trailer (chunked encoding).
+ * Returns true if the context has a valid algorithm and the location is set to trailer.
+ *
+ * @param context The checksum context to check
+ * @return true if checksum should be added as trailer, false otherwise
  */
 AWS_S3_API
 bool aws_s3_upload_request_checksum_context_should_add_trailer(
     const struct aws_s3_upload_request_checksum_context *context);
 
 /**
  * Get the checksum buffer to use for output.
- * Returns the user buffer if provided, otherwise the internal buffer.
+ * Returns the internal buffer for storing the calculated checksum.
+ *
+ * @param context The checksum context
+ * @return Pointer to the checksum buffer, or NULL if context is invalid
  */
 AWS_S3_API
 struct aws_byte_buf *aws_s3_upload_request_checksum_context_get_output_buffer(
     struct aws_s3_upload_request_checksum_context *context);
 
 /**
- * Validate that the checksum buffer size matches the expected size for the algorithm
- */
-AWS_S3_API
-int aws_s3_upload_request_checksum_context_validate_buffer_size(
-    const struct aws_s3_upload_request_checksum_context *context);
-
-/**
- * Get a cursor to the current checksum value (for use in headers/trailers)
+ * Get a cursor to the current checksum value (for use in headers/trailers).
+ * Returns an empty cursor if the checksum has not been calculated yet.
+ *
+ * @param context The checksum context
+ * @return Byte cursor to the calculated checksum, or empty cursor if not available
  */
 AWS_S3_API
 struct aws_byte_cursor aws_s3_upload_request_checksum_context_get_checksum_cursor(
diff --git a/include/aws/s3/private/s3_checksums.h b/include/aws/s3/private/s3_checksums.h
@@ -192,15 +192,15 @@ AWS_S3_API
 int aws_checksum_finalize(struct aws_s3_checksum *checksum, struct aws_byte_buf *output);
 
 AWS_S3_API
-int aws_aws_s3_meta_request_checksum_config_storage_init(
+int aws_s3_meta_request_checksum_config_storage_init(
     struct aws_allocator *allocator,
     struct aws_s3_meta_request_checksum_config_storage *internal_config,
     const struct aws_s3_checksum_config *config,
     const struct aws_http_message *message,
     const void *log_id);
 
 AWS_S3_API
-void aws_aws_s3_meta_request_checksum_config_storage_cleanup(
+void aws_s3_meta_request_checksum_config_storage_cleanup(
     struct aws_s3_meta_request_checksum_config_storage *internal_config);
 
 #endif /* AWS_S3_CHECKSUMS_H */
diff --git a/source/s3_auto_ranged_put.c b/source/s3_auto_ranged_put.c
@@ -155,13 +155,16 @@ static int s_process_part_info_synced(const struct aws_s3_part_info *info, void
 
     if ((checksum_cur != NULL) && (checksum_cur->len > 0)) {
         /* Create checksum context with pre-calculated checksum */
-        part->checksum_context = aws_s3_upload_request_checksum_context_new_with_exist_checksum(
+        part->checksum_context = aws_s3_upload_request_checksum_context_new_with_existing_checksum(
             auto_ranged_put->base.allocator, &auto_ranged_put->base.checksum_config, *checksum_cur);
     } else {
         part->checksum_context = aws_s3_upload_request_checksum_context_new(
             auto_ranged_put->base.allocator, &auto_ranged_put->base.checksum_config);
     }
-
+    if (!part->checksum_context) {
+        aws_mem_release(meta_request->allocator, part);
+        return AWS_OP_ERR;
+    }
     /* Parts might be out of order or have gaps in them.
      * Resize array-list to be long enough to hold this part,
      * filling any intermediate slots with NULL. */
@@ -1095,14 +1098,14 @@ static void s_s3_prepare_upload_part_on_read_done(void *user_data) {
                 (void *)meta_request);
             goto on_done;
         }
+        struct aws_s3_upload_request_checksum_context *context = previously_uploaded_info->checksum_context;
         /* if previously uploaded part had a checksum, compare it to what we just skipped */
-        if (previously_uploaded_info->checksum_context != NULL &&
-            previously_uploaded_info->checksum_context->checksum_calculated == true &&
+        if (context != NULL && context->checksum_calculated == true &&
             s_verify_part_matches_checksum(
                 meta_request->allocator,
                 aws_byte_cursor_from_buf(&request->request_body),
                 meta_request->checksum_config.checksum_algorithm,
-                aws_byte_cursor_from_buf(&previously_uploaded_info->checksum_context->base64_checksum))) {
+                aws_byte_cursor_from_buf(&context->base64_checksum))) {
             error_code = aws_last_error_or_unknown();
             goto on_done;
         }
diff --git a/source/s3_checksum_context.c b/source/s3_checksum_context.c
@@ -59,27 +59,27 @@ struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_co
     return context;
 }
 
-struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_context_new_with_exist_checksum(
+struct aws_s3_upload_request_checksum_context *aws_s3_upload_request_checksum_context_new_with_existing_checksum(
     struct aws_allocator *allocator,
     const struct aws_s3_meta_request_checksum_config_storage *checksum_config,
-    struct aws_byte_cursor exist_checksum) {
+    struct aws_byte_cursor existing_checksum) {
     struct aws_s3_upload_request_checksum_context *context =
         s_s3_upload_request_checksum_context_new_base(allocator, checksum_config);
     if (context) {
         /* Initial the buffer for checksum from the exist checksum */
-        if (context->encoded_checksum_size != exist_checksum.len) {
+        if (context->encoded_checksum_size != existing_checksum.len) {
             struct aws_byte_cursor algo_name = aws_get_checksum_algorithm_name(context->algorithm);
             AWS_LOGF_ERROR(
                 AWS_LS_S3_GENERAL,
                 "Encoded checksum size mismatch during creating the context for algorithm " PRInSTR
                 ": expected %zu bytes, got %zu bytes",
                 AWS_BYTE_CURSOR_PRI(algo_name),
                 context->encoded_checksum_size,
-                exist_checksum.len);
+                existing_checksum.len);
             aws_s3_upload_request_checksum_context_release(context);
             return NULL;
         }
-        aws_byte_buf_init_copy_from_cursor(&context->base64_checksum, allocator, exist_checksum);
+        aws_byte_buf_init_copy_from_cursor(&context->base64_checksum, allocator, existing_checksum);
         context->checksum_calculated = true;
     }
     return context;
diff --git a/source/s3_checksums.c b/source/s3_checksums.c
@@ -394,7 +394,7 @@ static int s_init_and_verify_checksum_config_from_headers(
     return AWS_OP_SUCCESS;
 }
 
-int aws_aws_s3_meta_request_checksum_config_storage_init(
+int aws_s3_meta_request_checksum_config_storage_init(
     struct aws_allocator *allocator,
     struct aws_s3_meta_request_checksum_config_storage *internal_config,
     const struct aws_s3_checksum_config *config,
@@ -488,7 +488,7 @@ int aws_aws_s3_meta_request_checksum_config_storage_init(
     return AWS_OP_SUCCESS;
 }
 
-void aws_aws_s3_meta_request_checksum_config_storage_cleanup(
+void aws_s3_meta_request_checksum_config_storage_cleanup(
     struct aws_s3_meta_request_checksum_config_storage *internal_config) {
     if (internal_config->has_full_object_checksum) {
         aws_byte_buf_clean_up(&internal_config->full_object_checksum);
diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c
@@ -228,7 +228,7 @@ int aws_s3_meta_request_init_base(
 
     *((size_t *)&meta_request->part_size) = part_size;
     *((bool *)&meta_request->should_compute_content_md5) = should_compute_content_md5;
-    if (aws_aws_s3_meta_request_checksum_config_storage_init(
+    if (aws_s3_meta_request_checksum_config_storage_init(
             meta_request->allocator,
             &meta_request->checksum_config,
             options->checksum_config,
@@ -500,7 +500,7 @@ static void s_s3_meta_request_destroy(void *user_data) {
     AWS_LOGF_DEBUG(AWS_LS_S3_META_REQUEST, "id=%p Cleaning up meta request", (void *)meta_request);
 
     /* Clean up our initial http message */
-    aws_aws_s3_meta_request_checksum_config_storage_cleanup(&meta_request->checksum_config);
+    aws_s3_meta_request_checksum_config_storage_cleanup(&meta_request->checksum_config);
     meta_request->request_body_async_stream = aws_async_input_stream_release(meta_request->request_body_async_stream);
     meta_request->initial_request_message = aws_http_message_release(meta_request->initial_request_message);
 
diff --git a/tests/s3_checksum_context_test.c b/tests/s3_checksum_context_test.c
@@ -64,7 +64,7 @@ static int s_test_upload_request_checksum_context_get_checksum_cursor(struct aws
 
     /* Test get checksum cursor with context that has calculated checksum */
     struct aws_byte_cursor existing_checksum = aws_byte_cursor_from_c_str("dGVzdA==");
-    context = aws_s3_upload_request_checksum_context_new_with_exist_checksum(allocator, &config, existing_checksum);
+    context = aws_s3_upload_request_checksum_context_new_with_existing_checksum(allocator, &config, existing_checksum);
     ASSERT_NOT_NULL(context);
 
     cursor = aws_s3_upload_request_checksum_context_get_checksum_cursor(context);
@@ -98,7 +98,7 @@ static int s_test_upload_request_checksum_context_error_cases(struct aws_allocat
     /* Test creation with mismatched checksum size */
     struct aws_byte_cursor wrong_size_checksum = aws_byte_cursor_from_c_str("short");
     struct aws_s3_upload_request_checksum_context *context =
-        aws_s3_upload_request_checksum_context_new_with_exist_checksum(allocator, &config, wrong_size_checksum);
+        aws_s3_upload_request_checksum_context_new_with_existing_checksum(allocator, &config, wrong_size_checksum);
     ASSERT_NULL(context);
 
     /* Test helper functions with NULL context */
