Repository: Unidata/netcdf-c
Pull request #1463 (2019-08-13 17:21:08)

more unit tests for nclistmgr.c
Part of #1458 

Added more tests and code comments. This replaces my previous PR so I will take that down once this one passes Travis.




Patch:
diff --git a/Makefile.am b/Makefile.am
index 876725885..61d3f523e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -21,6 +21,11 @@ test-driver-verbose test_common.in
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = netcdf.pc
 
+# Does the user want to build and run unit tests?
+if BUILD_UNIT_TESTS
+UNIT_TEST = unit_test
+endif # BUILD_UNIT_TESTS
+
 # Does the user want to build the V2 API?
 if BUILD_V2
 V2_TEST = nctest
@@ -85,8 +90,8 @@ endif # BUILD_BENCHMARKS
 
 # Define Test directories
 if BUILD_TESTSETS
-TESTDIRS = $(V2_TEST) nc_test $(NC_TEST4) $(BENCHMARKS_DIR)	\
-$(HDF4_TEST_DIR) $(NCDAP2TESTDIR) $(NCDAP4TESTDIR)
+TESTDIRS = $(UNIT_TEST) $(V2_TEST) nc_test $(NC_TEST4)			\
+$(BENCHMARKS_DIR) $(HDF4_TEST_DIR) $(NCDAP2TESTDIR) $(NCDAP4TESTDIR)
 endif
 
 # This is the list of subdirs for which Makefiles will be constructed
diff --git a/configure.ac b/configure.ac
index f311281ee..8ecd51400 100644
--- a/configure.ac
+++ b/configure.ac
@@ -184,6 +184,14 @@ if test "x$enable_jna" = xyes ; then
 AC_DEFINE([JNA], [1], [if true, include jna bug workaround code])
 fi
 
+# Does the user want to turn off unit tests (useful for test coverage
+# analysis).
+AC_ARG_ENABLE([unit-tests],
+  [AS_HELP_STRING([--disable-unit-tests],
+    [Disable tests in unit_test directory. Other tests still run.])])
+test "x$enable_unit_tests" = xno || enable_unit_tests=yes
+AM_CONDITIONAL([BUILD_UNIT_TESTS], [test "x$enable_unit_tests" = xyes])
+
 # Does the user want to build netcdf-4?
 AC_MSG_CHECKING([whether we should build netCDF-4])
 AC_ARG_ENABLE([netcdf-4], [AS_HELP_STRING([--disable-netcdf-4],
@@ -1536,6 +1544,7 @@ AC_CONFIG_FILES([Makefile
                  ncdump/expected/Makefile
                  docs/Makefile
                  docs/images/Makefile
+                 unit_test/Makefile
                  nctest/Makefile
                  nc_test4/Makefile
                  nc_perf/Makefile
diff --git a/libdispatch/nc.c b/libdispatch/nc.c
index ba07c6538..9e89d365b 100644
--- a/libdispatch/nc.c
+++ b/libdispatch/nc.c
@@ -1,7 +1,16 @@
 /*
- *	Copyright 2018, University Corporation for Atmospheric Research
+ *      Copyright 2018, University Corporation for Atmospheric Research
  *      See netcdf/COPYRIGHT file for copying and redistribution conditions.
  */
+/**
+ * @file
+ * @internal
+ *
+ * This file contains functions that work with the NC struct. There is
+ * an NC struct for every open netCDF file.
+ *
+ * @author Glenn Davis
+ */
 
 #include "config.h"
 #include <stdlib.h>
@@ -14,18 +23,21 @@
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
 #endif
-
 #include "ncdispatch.h"
 
-/* This is the default create format for nc_create and nc__create. */
+/** This is the default create format for nc_create and nc__create. */
 static int default_create_format = NC_FORMAT_CLASSIC;
 
-/* These have to do with version numbers. */
-#define MAGIC_NUM_LEN 4
-#define VER_CLASSIC 1
-#define VER_64BIT_OFFSET 2
-#define VER_HDF5 3
-
+/**
+ * Find the NC struct for an open file, using the ncid.
+ *
+ * @param ncid The ncid to find.
+ * @param ncpp Pointer that gets a pointer to the NC.
+ *
+ * @return ::NC_NOERR No error.
+ * @return ::NC_EBADID ncid not found.
+ * @author Glenn Davis, Dennis Heimbigner
+ */
 int
 NC_check_id(int ncid, NC** ncpp)
 {
@@ -35,15 +47,24 @@ NC_check_id(int ncid, NC** ncpp)
     return NC_NOERR;
 }
 
+/**
+ * Free an NC struct and its related resources. Before this is done,
+ * be sure to remove the NC from the open file list with
+ * del_from_NCList().
+ *
+ * @param ncp Pointer to the NC struct to be freed.
+ *
+ * @author Glenn Davis, Dennis Heimbigner
+ */
 void
 free_NC(NC *ncp)
 {
     if(ncp == NULL)
-	return;
+        return;
     if(ncp->path)
-	free(ncp->path);
+        free(ncp->path);
     if(ncp->model)
-	free(ncp->model);
+        free(ncp->model);
     /* We assume caller has already cleaned up ncp->dispatchdata */
 #if _CRAYMPP && defined(LOCKNUMREC)
     shfree(ncp);
@@ -52,8 +73,23 @@ free_NC(NC *ncp)
 #endif /* _CRAYMPP && LOCKNUMREC */
 }
 
+/**
+ * Create and initialize a new NC struct. The ncid is assigned later.
+ *
+ * @param dispatcher
+ * @param path The name of the file.
+ * @param mode The open or create mode.
+ * @param model
+ * @param ncpp A pointer that gets a pointer to the newlly allocacted
+ * and initialized NC struct.
+ *
+ * @return ::NC_NOERR No error.
+ * @return ::NC_ENOMEM Out of memory.
+ * @author Glenn Davis, Dennis Heimbigner
+ */
 int
-new_NC(const NC_Dispatch* dispatcher, const char* path, int mode, NCmodel* model, NC** ncpp)
+new_NC(const NC_Dispatch* dispatcher, const char* path, int mode,
+       NCmodel* model, NC** ncpp)
 {
     NC *ncp = (NC*)calloc(1,sizeof(NC));
     if(ncp == NULL) return NC_ENOMEM;
@@ -61,31 +97,40 @@ new_NC(const NC_Dispatch* dispatcher, const char* path, int mode, NCmodel* model
     ncp->path = nulldup(path);
     ncp->mode = mode;
     if((ncp->model = malloc(sizeof(NCmodel)))==NULL)
-	return NC_ENOMEM;
+        return NC_ENOMEM;
     *ncp->model = *model; /* Make a copy */
     if(ncp->path == NULL) { /* fail */
         free_NC(ncp);
-	return NC_ENOMEM;
+        return NC_ENOMEM;
     }
     if(ncpp) {
-      *ncpp = ncp;
+        *ncpp = ncp;
     } else {
-      free_NC(ncp);
+        free_NC(ncp);
     }
     return NC_NOERR;
 }
 
-/* This function sets a default create flag that will be logically
-   or'd to whatever flags are passed into nc_create for all future
-   calls to nc_create.
-   Valid default create flags are NC_64BIT_OFFSET, NC_CLOBBER,
-   NC_LOCK, NC_SHARE. */
+/**
+ * This function sets a default create flag that will be logically
+ *  or'd to whatever flags are passed into nc_create for all future
+ *  calls to nc_create.
+ *
+ * @param format The format that should become the default.
+ * @param old_formatp Pointer that gets the previous default. Ignored
+ * if NULL.
+ *
+ * @return ::NC_NOERR No error.
+ * @return ::NC_ENOTBUILD Requested format not built with this install.
+ * @return ::NC_EINVAL Invalid input.
+ * @author Ed Hartnett, Dennis Heimbigner
+ */
 int
 nc_set_default_format(int format, int *old_formatp)
 {
     /* Return existing format if desired. */
     if (old_formatp)
-      *old_formatp = default_create_format;
+        *old_formatp = default_create_format;
 
     /* Make sure only valid format is set. */
 #ifndef ENABLE_CDF5
@@ -95,7 +140,7 @@ nc_set_default_format(int format, int *old_formatp)
 #ifdef USE_HDF5
     if (format != NC_FORMAT_CLASSIC && format != NC_FORMAT_64BIT_OFFSET &&
         format != NC_FORMAT_NETCDF4 && format != NC_FORMAT_NETCDF4_CLASSIC &&
-	format != NC_FORMAT_CDF5)
+        format != NC_FORMAT_CDF5)
         return NC_EINVAL;
 #else
     if (format == NC_FORMAT_NETCDF4 || format == NC_FORMAT_NETCDF4_CLASSIC)
@@ -108,6 +153,12 @@ nc_set_default_format(int format, int *old_formatp)
     return NC_NOERR;
 }
 
+/**
+ * Get the current default format.
+ *
+ * @return the default format.
+ * @author Ed Hartnett
+ */
 int
 nc_get_default_format(void)
 {
diff --git a/libdispatch/nclistmgr.c b/libdispatch/nclistmgr.c
index 043addb6e..e05b7a3a4 100644
--- a/libdispatch/nclistmgr.c
+++ b/libdispatch/nclistmgr.c
@@ -1,7 +1,15 @@
 /*********************************************************************
    Copyright 2018, UCAR/Unidata See netcdf/COPYRIGHT file for
    copying and redistribution conditions.
- *********************************************************************/
+*********************************************************************/
+/**
+ * @file
+ *
+ * Functions to manage the list of NC structs. There is one NC struct
+ * for each open file.
+ *
+ * @author Dennis Heimbigner
+*/
 
 #include "config.h"
 #include <stdlib.h>
@@ -9,22 +17,40 @@
 #include <assert.h>
 #include "ncdispatch.h"
 
+/** This shift is applied to the ext_ncid in order to get the index in
+ * the array of NC. */
 #define ID_SHIFT (16)
+
+/** This is the length of the NC list - the number of files that can
+ * be open at one time. We use 2^16 = 65536 entries in the array, but
+ * slot 0 is not used, so only 65535 files may be open at one
+ * time. */
 #define NCFILELISTLENGTH 0x10000
 
-/* Version one just allocates the max space (sizeof(NC*)*2^16)*/
+/** This is the pointer to the array of NC, one for each open file. */
 static NC** nc_filelist = NULL;
 
+/** The number of files currently open. */
 static int numfiles = 0;
 
-/* Common */
+/**
+ * How many files are currently open?
+ *
+ * @return number of open files.
+ * @author Dennis Heimbigner
+ */
 int
 count_NCList(void)
 {
     return numfiles;
 }
 
-
+/**
+ * Free an empty NCList. @note If list is not empty, or has not been
+ * allocated, function will silently exit.
+ *
+ * @author Dennis Heimbigner
+ */
 void
 free_NCList(void)
 {
@@ -33,86 +59,156 @@ free_NCList(void)
     nc_filelist = NULL;
 }
 
+/**
+ * Add an already-allocated NC to the list. It will be assigned an
+ * ncid in this function.
+ *
+ * If this is the first file to be opened, the nc_filelist will be
+ * allocated and set to all 0.
+ *
+ * The ncid is assigned by finding the first open index in the
+ * nc_filelist array (skipping index 0). The ncid is this index
+ * left-shifted ID_SHIFT bits (16). This puts the file ID in the first
+ * two bytes of the 4-byte integer, and leaves the last two bytes for
+ * group IDs for netCDF-4 files.
+ *
+ * @param ncp Pointer to already-allocated and initialized NC struct.
+ *
+ * @return ::NC_NOERR No error.
+ * @return ::NC_ENOMEM Out of memory.
+ * @author Dennis Heimbigner
+ */
 int
 add_to_NCList(NC* ncp)
 {
     int i;
     int new_id;
     if(nc_filelist == NULL) {
-	if (!(nc_filelist = calloc(1, sizeof(NC*)*NCFILELISTLENGTH)))
-	    return NC_ENOMEM;
-	numfiles = 0;
+        if (!(nc_filelist = calloc(1, sizeof(NC*)*NCFILELISTLENGTH)))
+            return NC_ENOMEM;
+        numfiles = 0;
     }
 
     new_id = 0; /* id's begin at 1 */
     for(i=1; i < NCFILELISTLENGTH; i++) {
-	if(nc_filelist[i] == NULL) {new_id = i; break;}
+        if(nc_filelist[i] == NULL) {new_id = i; break;}
     }
     if(new_id == 0) return NC_ENOMEM; /* no more slots */
     nc_filelist[new_id] = ncp;
     numfiles++;
-    new_id = (new_id << ID_SHIFT);
-    ncp->ext_ncid = new_id;
+    ncp->ext_ncid = (new_id << ID_SHIFT);
     return NC_NOERR;
 }
 
+/**
+ * Delete an NC struct from the list. This happens when the file is
+ * closed. Relies on all memory in the NC being deallocated after this
+ * function with freeNC().
+ *
+ * @note If the file list is empty, or this NC can't be found in the
+ * list, this function will silently exit.
+ *
+ * @param ncp Pointer to NC to be removed from list.
+ *
+ * @author Dennis Heimbigner
+ */
 void
 del_from_NCList(NC* ncp)
 {
-   unsigned int ncid = ((unsigned int)ncp->ext_ncid) >> ID_SHIFT;
-   if(numfiles == 0 || ncid == 0 || nc_filelist == NULL) return;
-   if(nc_filelist[ncid] != ncp) return;
+    unsigned int ncid = ((unsigned int)ncp->ext_ncid) >> ID_SHIFT;
+    if(numfiles == 0 || ncid == 0 || nc_filelist == NULL) return;
+    if(nc_filelist[ncid] != ncp) return;
 
-   nc_filelist[ncid] = NULL;
-   numfiles--;
+    nc_filelist[ncid] = NULL;
+    numfiles--;
 
-   /* If all files have been closed, release the filelist memory. */
-   if (numfiles == 0)
-      free_NCList();
+    /* If all files have been closed, release the filelist memory. */
+    if (numfiles == 0)
+        free_NCList();
 }
 
+/**
+ * Find an NC in the list, given an ext_ncid. The NC list is indexed
+ * with the first two bytes of ext_ncid. (The last two bytes specify
+ * the group for netCDF4 files, or are zeros for classic files.)
+ *
+ * @param ext_ncid The ncid of the file to find.
+ *
+ * @return pointer to NC or NULL if not found.
+ * @author Dennis Heimbigner, Ed Hartnett
+ */
 NC *
 find_in_NCList(int ext_ncid)
 {
-   NC* f = NULL;
-   unsigned int ncid = ((unsigned int)ext_ncid) >> ID_SHIFT;
-   if(numfiles > 0 && nc_filelist != NULL && ncid < NCFILELISTLENGTH)
-	f = nc_filelist[ncid];
+    NC* f = NULL;
+    /* Discard the first two bytes of ext_ncid to get ncid. */
+    unsigned int ncid = ((unsigned int)ext_ncid) >> ID_SHIFT;
+
+    /* If we have a filelist, there will be an entry, possibly NULL,
+     * for this ncid. */
+    if (nc_filelist)
+    {
+        assert(numfiles);
+        f = nc_filelist[ncid];
+    }
 
-   /* for classic files, ext_ncid must be a multiple of (1<<ID_SHIFT) */
-   if (f != NULL && f->model->impl == NC_FORMATX_NC3 && (ext_ncid % (1<<ID_SHIFT)))
-       return NULL;
+    /* For classic files, ext_ncid must be a multiple of
+     * (1<<ID_SHIFT). That is, the group part of the ext_ncid (the
+     * last two bytes) must be zero. If not, then return NULL, which
+     * will eventually lead to an NC_EBADID error being returned to
+     * user. */
+    if (f != NULL && f->model->impl == NC_FORMATX_NC3 && (ext_ncid % (1<<ID_SHIFT)))
+        return NULL;
 
-   return f;
+    return f;
 }
 
-/*
-Added to support open by name
-*/
+/**
+ * Find an NC in the list using the file name.
+ *
+ * @param path Name of the file.
+ *
+ * @return pointer to NC or NULL if not found.
+ * @author Dennis Heimbigner
+ */
 NC*
 find_in_NCList_by_name(const char* path)
 {
-   int i;
-   NC* f = NULL;
-   if(nc_filelist == NULL)
-	return NULL;
-   for(i=1; i < NCFILELISTLENGTH; i++) {
-	if(nc_filelist[i] != NULL) {
-	    if(strcmp(nc_filelist[i]->path,path)==0) {
-		f = nc_filelist[i];
-		break;
-	    }
-	}
-   }
-   return f;
+    int i;
+    NC* f = NULL;
+    if(nc_filelist == NULL)
+        return NULL;
+    for(i=1; i < NCFILELISTLENGTH; i++) {
+        if(nc_filelist[i] != NULL) {
+            if(strcmp(nc_filelist[i]->path,path)==0) {
+                f = nc_filelist[i];
+                break;
+            }
+        }
+    }
+    return f;
 }
 
+/**
+ * Find an NC in list based on its index. The index is ((unsigned
+ * int)ext_ncid) >> ID_SHIFT. This is the two high bytes of the
+ * ext_ncid. (The other two bytes are used for the group ID for
+ * netCDF-4 files.)
+ *
+ * @param index The index in the NC list.
+ * @param ncp Pointer that gets pointer to the next NC. Igored if
+ * NULL.
+ *
+ * @return ::NC_NOERR No error.
+ * @return ::NC_ERANGE Index out of range.
+ * @author Dennis Heimbigner
+ */
 int
 iterate_NCList(int index, NC** ncp)
 {
     /* Walk from 0 ...; 0 return => stop */
     if(index < 0 || index >= NCFILELISTLENGTH)
-	return NC_ERANGE;
+        return NC_ERANGE;
     if(ncp) *ncp = nc_filelist[index];
     return NC_NOERR;
 }


Test patch:
diff --git a/unit_test/Makefile.am b/unit_test/Makefile.am
new file mode 100644
index 000000000..c83024a77
--- /dev/null
+++ b/unit_test/Makefile.am
@@ -0,0 +1,21 @@
+## This is a automake file, part of Unidata's netCDF package.
+# Copyright 2019, see the COPYRIGHT file for more information.
+
+# This file builds and runs the unit tests. These tests are not run in
+# the CMake build, because we would then have to extern these internal
+# functions, to allow Windows to work. Since we have not extern'd
+# these functions, they will only be run under the autotools build.
+
+# Ed Hartnett 8/9/19
+
+# Put together AM_CPPFLAGS and AM_LDFLAGS.
+include $(top_srcdir)/lib_flags.am
+
+# Find and link to the netcdf-c library.
+LDADD = ${top_builddir}/liblib/libnetcdf.la
+
+check_PROGRAMS = tst_nclist
+TESTS = tst_nclist
+
+# If valgrind is present, add valgrind targets.
+@VALGRIND_CHECK_RULES@
diff --git a/unit_test/tst_nclist.c b/unit_test/tst_nclist.c
new file mode 100644
index 000000000..3637a1642
--- /dev/null
+++ b/unit_test/tst_nclist.c
@@ -0,0 +1,110 @@
+/* This is part of the netCDF package. Copyright 2005-2019 University
+   Corporation for Atmospheric Research/Unidata. See COPYRIGHT file
+   for conditions of use.
+
+   Test list functions in nclistmgr.c.
+
+   Ed Hartnett, 8/10/19
+*/
+
+#include "config.h"
+#include <nc_tests.h>
+#include "nc.h"
+#include "ncdispatch.h"
+#include "err_macros.h"
+
+/* An integer value to use in testing. */
+#define TEST_VAL_42 42
+
+int
+main(int argc, char **argv)
+{
+    printf("\n*** Testing netcdf internal NC list functions.\n");
+    printf("Testing NC list functions with no open files...");
+    {
+        /* These are cases where there are no files, or NULL
+         * params. */
+        if (count_NCList()) ERR;
+        free_NCList();
+        if (find_in_NCList_by_name("nope")) ERR;
+        if (iterate_NCList(0, NULL)) ERR;
+    }
+    SUMMARIZE_ERR;
+    printf("Testing adding to NC list...");
+    {
+        int ncid;
+        NC *ncp, *ncp2;
+        int mode = 0;
+        NCmodel model;
+        char path[] = {"file.nc"};
+        int ret;
+
+        /* Create the NC* instance and insert its dispatcher and model. */
+        if ((ret = new_NC(NULL, path, mode, &model, &ncp))) ERR;
+
+        /* Nothing to find yet. */
+        if (find_in_NCList(TEST_VAL_42)) ERR;
+
+        /* Add to list of known open files and define ext_ncid. */
+        add_to_NCList(ncp);
+
+        /* These won't work! */
+        if (find_in_NCList(TEST_VAL_42)) ERR;
+        if (find_in_NCList_by_name("nope")) ERR;
+        if ((ret = iterate_NCList(2, &ncp2))) ERR;
+
+        /* Find it in the list. */
+        if (!(ncp2 = find_in_NCList(ncp->ext_ncid))) ERR;
+        if (!(ncp2 = find_in_NCList_by_name(path))) ERR;
+        if ((ret = iterate_NCList(1, &ncp2))) ERR;
+        if (count_NCList() != 1) ERR;
+
+        /* Won't do anything because list contains an entry. */
+        free_NCList();
+
+        /* Delete it. */
+        ncid = ncp->ext_ncid;
+        del_from_NCList(ncp); /* Will free empty list. */
+        free_NC(ncp);
+        if (find_in_NCList(ncid)) ERR;
+    }
+    SUMMARIZE_ERR;
+#ifdef LARGE_FILE_TESTS
+    /* This test is slow, only run it on large file test builds. */
+    printf("Testing maxing out NC list...");
+    {
+        NC *ncp;
+        int mode = 0;
+        NCmodel model;
+        char path[] = {"file.nc"};
+        int max_num_nc = 65535;
+        int i;
+        int ret;
+
+        /* Fill the NC list. */
+        for (i = 0; i < max_num_nc; i++)
+        {
+            if ((ret = new_NC(NULL, path, mode, &model, &ncp))) ERR;
+            if (add_to_NCList(ncp)) ERR;
+        }
+
+        /* Check the count. */
+        if (count_NCList() != max_num_nc) ERR;
+
+        /* Try and add another. It will fail. */
+        if (add_to_NCList(ncp) != NC_ENOMEM) ERR;
+
+        /* Delete them all. */
+        for (i = 0; i < max_num_nc; i++)
+        {
+            if (iterate_NCList(i + 1, &ncp)) ERR;
+            if (!ncp) ERR;
+            del_from_NCList(ncp);
+            free_NC(ncp);
+        }
+    }
+    SUMMARIZE_ERR;
+#endif /* LARGE_FILE_TESTS */
+
+    FINAL_RESULTS;
+}