Rework: platform_buffer_deinit() can only be called on bh that was su…

…ccessfully init()'ed previously.

Fix error handling in clockcache_init() -> deinit(), to now only call
platform_buffer_deinit() on cc->variables that are validly pointing to
buffer handle addresses that were previously successfully init()'ed.

This addresses Rob's last remaining comment on the error handling aspects
of this fix.

Adjust unit-tests accordingly. Backout prelim version of clockcache_test.c
as it was felt to be not necessary at this time.

Minor bugfix: Correct use of uint32 to uint64 to avoid overflow.

Makefile

@@ -446,10 +446,6 @@ $(BINDIR)/$(UNITDIR)/task_system_test: $(UTIL_SYS)
 $(BINDIR)/$(UNITDIR)/platform_apis_test: $(UTIL_SYS)       \
                                          $(PLATFORM_SYS)
 
-$(BINDIR)/$(UNITDIR)/clockcache_test: $(CLOCKCACHE_SYS)               \
-                                      $(OBJDIR)/$(TESTS_DIR)/config.o \
-                                      $(UTIL_SYS)
-
 ########################################
 # Convenience mini unit-test targets
 unit/util_test:                    $(BINDIR)/$(UNITDIR)/util_test

src/clockcache.c

@@ -1902,16 +1902,21 @@ clockcache_deinit(clockcache *cc) // IN/OUT
       platform_free(cc->heap_id, cc->entry);
    }
 
-   platform_buffer_deinit(&cc->bh);
-   platform_buffer_deinit(&cc->rc_bh);
+   if (cc->data) {
+      platform_buffer_deinit(&cc->bh);
+      cc->data = NULL;
+   }
+   if (cc->refcount) {
+      platform_buffer_deinit(&cc->rc_bh);
+      cc->refcount = NULL;
+   }
 
    if (cc->pincount) {
       platform_free_volatile(cc->heap_id, cc->pincount);
    }
    if (cc->batch_busy) {
       platform_free_volatile(cc->heap_id, cc->batch_busy);
    }
-   ZERO_CONTENTS(cc);
 }
 
 /*

src/platform_linux/platform.c

@@ -110,19 +110,19 @@ platform_buffer_getaddr(const buffer_handle *bh)
 /*
  * platform_buffer_deinit() - Deinit the buffer handle, which involves
  * unmapping the memory for the large buffer created using mmap().
+ * This is expected to be called on a 'bh' that has been successfully
+ * initialized, thru a prior platform_buffer_init() call.
  */
 platform_status
 platform_buffer_deinit(buffer_handle *bh)
 {
-   if (bh->addr) {
-      int ret;
-      ret = munmap(bh->addr, bh->length);
-      if (ret) {
-         return CONST_STATUS(errno);
-      }
-
-      bh->addr = NULL;
+   int ret;
+   ret = munmap(bh->addr, bh->length);
+   if (ret) {
+      return CONST_STATUS(errno);
    }
+
+   bh->addr   = NULL;
    bh->length = 0;
    return STATUS_OK;
 }

src/rc_allocator.c

@@ -434,7 +434,7 @@ rc_allocator_mount(rc_allocator      *al,
    platform_assert(cfg->capacity
                    == cfg->io_cfg->page_size * cfg->page_capacity);
 
-   uint32 buffer_size = cfg->extent_capacity * sizeof(uint8);
+   uint64 buffer_size = cfg->extent_capacity * sizeof(uint8);
    buffer_size        = ROUNDUP(buffer_size, cfg->io_cfg->page_size);
    status             = platform_buffer_init(&al->bh, buffer_size);
    if (!SUCCESS(status)) {

tests/unit/platform_apis_test.c

@@ -89,13 +89,16 @@ CTEST2(platform_api, test_platform_buffer_init_fails_for_very_large_length)
 
    size_t length = (1024 * KiB * GiB);
 
-   // This is expected to fail as length is very large
+   // On most test machines we use, this is expected to fail as mmap() under
+   // here will fail for very large lengths. (If this test case ever fails,
+   // check the 'length' here and the machine's configuration to see why
+   // mmap() unexpectedly succeeded.)
    rc = platform_buffer_init(&bh, length);
    ASSERT_FALSE(SUCCESS(rc));
    ASSERT_TRUE(bh.addr == NULL);
    ASSERT_TRUE(bh.length == 0);
 
-   // deinit() should still work even when nothing was successfully mmap()'ed
+   // deinit() would fail horribly when nothing was successfully mmap()'ed
    rc = platform_buffer_deinit(&bh);
-   ASSERT_TRUE(SUCCESS(rc), "rc='%s'", platform_status_to_string(rc));
+   ASSERT_FALSE(SUCCESS(rc));
 }