Addressed PR feedback, rewrote the file to buffer API and added a tes…

…t (there wasn't one).

bin/system_info/print_system_info.c

@@ -1,9 +1,8 @@
 
 
-
-#include <aws/common/system_info.h>
 #include <aws/common/byte_buf.h>
 #include <aws/common/logging.h>
+#include <aws/common/system_info.h>
 
 int main(void) {
     struct aws_allocator *allocator = aws_default_allocator();
@@ -22,10 +21,15 @@ int main(void) {
     fprintf(stdout, "crt-detected env: {\n");
 
     struct aws_byte_cursor virtualization_vendor = aws_system_environment_get_virtualization_vendor(env);
-    fprintf(stdout, "  'virtualization vendor': '" PRInSTR "',\n", (int)virtualization_vendor.len, virtualization_vendor.ptr);
+    fprintf(
+        stdout,
+        "  'virtualization vendor': '" PRInSTR "',\n",
+        (int)virtualization_vendor.len,
+        virtualization_vendor.ptr);
     struct aws_byte_cursor product_name = aws_system_environment_get_virtualization_product_name(env);
     fprintf(stdout, "  'product name': '" PRInSTR "',\n", (int)product_name.len, product_name.ptr);
-    fprintf(stdout, "  'number of processors': '%lu',\n", (unsigned long)aws_system_environment_get_processor_count(env));
+    fprintf(
+        stdout, "  'number of processors': '%lu',\n", (unsigned long)aws_system_environment_get_processor_count(env));
     size_t numa_nodes = aws_system_environment_get_cpu_group_count(env);
 
     if (numa_nodes > 1) {
@@ -36,7 +40,7 @@ int main(void) {
     }
 
     fprintf(stdout, "}\n");
-    aws_system_environment_destroy(env);
+    aws_system_environment_release(env);
     aws_logger_clean_up(&logger);
 
     aws_common_library_clean_up();

include/aws/common/private/system_info_priv.h

@@ -4,13 +4,14 @@
  * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
  * SPDX-License-Identifier: Apache-2.0.
  */
-
 #include <aws/common/byte_buf.h>
+#include <aws/common/ref_count.h>
 #include <aws/common/string.h>
 #include <aws/common/system_info.h>
 
 struct aws_system_environment {
     struct aws_allocator *allocator;
+    struct aws_ref_count ref_count;
     struct aws_byte_buf virtualization_vendor;
     struct aws_byte_buf product_name;
     enum aws_platform_os os;

include/aws/common/system_info.h

@@ -34,7 +34,10 @@ AWS_COMMON_API
 struct aws_system_environment *aws_system_environment_load(struct aws_allocator *allocator);
 
 AWS_COMMON_API
-void aws_system_environment_destroy(struct aws_system_environment *env);
+struct aws_system_environment *aws_system_environment_acquire(struct aws_system_environment *env);
+
+AWS_COMMON_API
+void aws_system_environment_release(struct aws_system_environment *env);
 
 /**
  * Returns the virtualization vendor for the specified compute environment, e.g. "Xen, Amazon EC2, etc..."
@@ -62,7 +65,7 @@ size_t aws_system_environment_get_processor_count(struct aws_system_environment
  * Returns the number of separate cpu groupings (multi-socket configurations or NUMA).
  */
 AWS_COMMON_API
-size_t aws_system_environment_get_cpu_group_count(struct aws_system_environment *env);
+size_t aws_system_environment_get_cpu_group_count(const struct aws_system_environment *env);
 
 /* Returns the OS this was built under */
 AWS_COMMON_API

source/file.c

@@ -62,39 +62,43 @@ int aws_byte_buf_init_from_file(struct aws_byte_buf *out_buf, struct aws_allocat
         goto error;
     }
 
+    /*
+     * This number is usually correct, but in cases of device files that don't correspond to storage on disk,
+     * it may just be the size of a page. Go ahead and use it as a good hint of how much to allocate initially,
+     * but otherwise don't rely on it.
+     */
     size_t allocation_size = (size_t)len64 + 1;
     aws_byte_buf_init(out_buf, alloc, allocation_size);
 
-    /* Ensure compatibility with null-terminated APIs, but don't consider
-     * the null terminator part of the length of the payload */
-    out_buf->len = out_buf->capacity - 1;
-    out_buf->buffer[out_buf->len] = 0;
-
-    size_t read = fread(out_buf->buffer, 1, out_buf->len, fp);
-    if (read < out_buf->len) {
-        int errno_value = ferror(fp) ? errno : 0; /* Always cache errno before potential side-effect */
-        if (errno_value != 0) {
-            aws_translate_and_raise_io_error_or(errno_value, AWS_ERROR_FILE_READ_FAILURE);
-            AWS_LOGF_ERROR(
-                AWS_LS_COMMON_IO,
-                "static: Failed reading file:'%s' errno:%d aws-error:%s",
-                filename,
-                errno_value,
-                aws_error_name(aws_last_error()));
-            goto error;
-        } else {
-            AWS_LOGF_WARN(
-                AWS_LS_COMMON_IO,
-                "static: reading file:'%s' reports longer length %d than actually reading from it: %d. This is not "
-                "necessarily an error as devices will usually report a page for length regardless of actual length.",
-                filename,
-                (int)read,
-                (int)out_buf->len);
-            out_buf->len = read;
+    size_t read = -1;
+    size_t total_read = 0;
+    do {
+        if (total_read == out_buf->capacity) {
+            /* just add allocation size space to read some more. It's not perfect but it's plenty good. */
+            aws_byte_buf_reserve_relative(out_buf, allocation_size);
         }
+        read = fread(out_buf->buffer + out_buf->len, 1, out_buf->capacity - out_buf->len, fp);
+        out_buf->len += read;
+        total_read += read;
+    } while (read > 0);
+
+    int errno_value = ferror(fp) ? errno : 0; /* Always cache errno before potential side-effect */
+    if (errno_value != 0) {
+        aws_translate_and_raise_io_error_or(errno_value, AWS_ERROR_FILE_READ_FAILURE);
+        AWS_LOGF_ERROR(
+            AWS_LS_COMMON_IO,
+            "static: Failed reading file:'%s' errno:%d aws-error:%s",
+            filename,
+            errno_value,
+            aws_error_name(aws_last_error()));
+        goto error;
     }
 
     fclose(fp);
+    /* write the NULL terminator out. */
+    aws_byte_buf_write_u8(out_buf, 0x00);
+    /* we wrote the NULL terminator, but don't include it in the length. */
+    out_buf->len -= 1;
     return AWS_OP_SUCCESS;
 
 error:

source/system_info.c

@@ -6,9 +6,19 @@
 
 #include <aws/common/logging.h>
 
+void s_destroy_env(void *arg) {
+    struct aws_system_environment *env = arg;
+
+    if (env) {
+        aws_system_environment_destroy_platform_impl(env);
+        aws_mem_release(env->allocator, env);
+    }
+}
+
 struct aws_system_environment *aws_system_environment_load(struct aws_allocator *allocator) {
     struct aws_system_environment *env = aws_mem_calloc(allocator, 1, sizeof(struct aws_system_environment));
     env->allocator = allocator;
+    aws_ref_count_init(&env->ref_count, env, s_destroy_env);
 
     if (aws_system_environment_load_platform_impl(env)) {
         AWS_LOGF_ERROR(
@@ -36,16 +46,17 @@ struct aws_system_environment *aws_system_environment_load(struct aws_allocator
 
     return env;
 error:
-    aws_mem_release(allocator, env);
-
+    s_destroy_env(env);
     return NULL;
 }
 
-void aws_system_environment_destroy(struct aws_system_environment *env) {
-    if (env) {
-        aws_system_environment_destroy_platform_impl(env);
-        aws_mem_release(env->allocator, env);
-    }
+struct aws_system_environment *aws_system_environment_acquire(struct aws_system_environment *env) {
+    aws_ref_count_acquire(&env->ref_count);
+    return env;
+}
+
+void aws_system_environment_release(struct aws_system_environment *env) {
+    aws_ref_count_release(&env->ref_count);
 }
 
 struct aws_byte_cursor aws_system_environment_get_virtualization_vendor(const struct aws_system_environment *env) {
@@ -64,6 +75,6 @@ size_t aws_system_environment_get_processor_count(struct aws_system_environment
 }
 
 AWS_COMMON_API
-size_t aws_system_environment_get_cpu_group_count(struct aws_system_environment *env) {
+size_t aws_system_environment_get_cpu_group_count(const struct aws_system_environment *env) {
     return env->cpu_group_count;
 }

tests/CMakeLists.txt

@@ -481,6 +481,7 @@ add_test_case(directory_move_src_non_existent_test)
 add_test_case(test_home_directory_not_null)
 add_test_case(test_normalize_posix_directory_separator)
 add_test_case(test_normalize_windows_directory_separator)
+add_test_case(test_byte_buf_file_read)
 
 add_test_case(promise_test_wait_forever)
 add_test_case(promise_test_wait_for_a_bit)

tests/file_test.c

@@ -439,3 +439,31 @@ static int s_test_normalize_windows_directory_separator(struct aws_allocator *al
 }
 
 AWS_TEST_CASE(test_normalize_windows_directory_separator, s_test_normalize_windows_directory_separator);
+
+static int s_test_byte_buf_file_read(struct aws_allocator *allocator, void *ctx) {
+    (void)ctx;
+
+    const char *test_string = "This is a message that's going to test a read loop.";
+    struct aws_byte_buf test_buf = aws_byte_buf_from_c_str(test_string);
+    struct aws_string *test_file = aws_string_new_from_c_str(allocator, "test_file.txt");
+    struct aws_string *test_file_mode = aws_string_new_from_c_str(allocator, "w");
+
+    FILE *output_file = aws_fopen("test_file", "w");
+    ASSERT_NOT_NULL(output_file);
+    ASSERT_UINT_EQUALS(test_buf.len, fwrite(test_buf.buffer, 1, test_buf.len, output_file));
+    fclose(output_file);
+
+    struct aws_byte_buf output_buf;
+    AWS_ZERO_STRUCT(output_buf);
+    ASSERT_SUCCESS(aws_byte_buf_init_from_file(&output_buf, allocator, "test_file"));
+    aws_file_delete(test_file);
+    ASSERT_BIN_ARRAYS_EQUALS(test_buf.buffer, test_buf.len, output_buf.buffer, output_buf.len);
+
+    aws_byte_buf_clean_up(&output_buf);
+    aws_string_destroy(test_file_mode);
+    aws_string_destroy(test_file);
+
+    return AWS_OP_SUCCESS;
+}
+
+AWS_TEST_CASE(test_byte_buf_file_read, s_test_byte_buf_file_read);

tests/system_info_tests.c

@@ -178,7 +178,7 @@ static int s_test_sanity_check_environment_loader(struct aws_allocator *allocato
     struct aws_byte_cursor virt_product = aws_system_environment_get_virtualization_product_name(env);
     ASSERT_TRUE(aws_byte_cursor_is_valid(&virt_product));
 
-    aws_system_environment_destroy(env);
+    aws_system_environment_release(env);
 
     aws_common_library_clean_up();
     return 0;