Allow redefinition of variable filters

re: Github issue Unidata#1713

If nc_def_var_filter or nc_def_var_deflate or nc_def_var_szip is
called multiple times with the same filter id, but possibly with
different sets of parameters, then the first invocation is
sticky and later invocations are ignored. The desired behavior
is to have the last invocation be used.

This PR implements that desired behavior, with some special
cases.  If you call nc_def_var_deflate multiple times, then the
last invocation rule applies with respect to deflate. However,
the shuffle filter, if enabled, is always applied just before
applying deflate.

Misc unrelated changes:
1. Make client-side filters be disabled by default
2. Fix the definition of uintptr_t and use in oc2 and libdap4
3. Add some test cases
4. modify filter order tests to use plugin filters rather
   than client-side filters

CMakeLists.txt

@@ -1182,6 +1182,11 @@ IF(NOT BUILD_SHARED_LIBS)
   MESSAGE(WARNING "ENABLE_FILTER_TESTING requires shared libraries. Disabling.")
   SET(ENABLE_FILTER_TESTING OFF)
 ENDIF()
+OPTION(ENABLE_CLIENTSIDE_FILTERS "Enable client-side filter registration." OFF)
+IF(NOT ENABLE_FILTER_TESTING)
+SET(ENABLE_CLIENTSIDE_FILTERS OFF)
+ENDIF()
+
 
 # Determine whether or not to generate documentation.
 OPTION(ENABLE_DOXYGEN "Enable generation of doxygen-based documentation." OFF)
@@ -1776,6 +1781,7 @@ ENDIF()
 add_subdirectory(liblib)
 
 IF(ENABLE_FILTER_TESTING)
+  CONFIGURE_FILE(plugins/H5Znoop.c ${CMAKE_SOURCE_DIR}/plugins/H5Znoop1.c COPYONLY)
   add_subdirectory(plugins)
 ENDIF()
 
@@ -2110,8 +2116,6 @@ install(
 # End export files
 ####
 
-
-
 # CPack inclusion must come last.
 # INCLUDE(CPack)
 INCLUDE(CMakeInstallation.cmake)

NUG/filters.md

@@ -482,6 +482,34 @@ demonstrate how to build the hdf5 plugin for bzip2.
 Notes
 ==========
 
+Order of Invocation for Multiple Filters 
+-----------
+
+When multiple filters are defined on a variable, the
+order of application, when writing data to the file,
+is same as the order in which _nc_def_var_filter_ is called.
+When reading a file the order of application is of necessity
+the reverse.
+
+There are some special cases.
+
+1. The fletcher32 filter is always applied first, if enabled.
+1. If _nc_def_var_filter_ or _nc_def_var_deflate_ or _nc_def_var_szip_
+is called multiple times with the same filter id, but possibly
+with different sets of parameters, then the position of that filter
+in the sequence of applictions does not change. However the last set
+of parameters specified is used when actually writing the dataset.
+1. Deflate and shuffle -- these two are inextricably linked in the
+current API, but have quite different semantics.
+If you call _nc_def_var_deflate_ multiple times, then 
+the previous rule applies with respect to deflate. However,
+the shuffle filter, if enabled, is ''always'' applied before
+applying any other filters, except fletcher32.
+1. If you want to move the location of a filter in the application
+sequence, then you must remove it using _nc_var_filter_remove_
+or using _nc_def_var_deflate_. The next time you add the filter
+back, its position will be at the end of the current sequence.
+
 Memory Allocation Issues
 -----------
 

RELEASE_NOTES.md

@@ -7,6 +7,9 @@ This file contains a high-level description of this package's evolution. Release
 
 ## 4.8.0 - TBD
 
+* [Enhancement] When a filter is applied twice with different
+parameters, then the second set is used for writing the dataset
+[https://github.com/Unidata/netcdf-c/issues/1713].
 * [Bug Fix] Now larger cache settings are used for sequential HDF5 file creates/opens on parallel I/O capable builds; see [Github #1716](https://github.com/Unidata/netcdf-c/issues/1716) for more information.
 * [Bug Fix] Add functions to libdispatch/dnotnc4.c to support
 dispatch table operations that should work for any dispatch

configure.ac

@@ -1428,6 +1428,20 @@ enable_filter_testing=no
 fi
 AM_CONDITIONAL(ENABLE_FILTER_TESTING, [test x$enable_filter_testing = xyes])
 
+# Enable client side filter registration
+AC_MSG_CHECKING([If client-side filters are enabled (default off)])
+AC_ARG_ENABLE([clientside-filters],
+              [AS_HELP_STRING([--enable-clientside-filters],
+                              [enable client side filters])],
+				[],
+				[enable_clientside_filters=no])
+test "x$enable_clientside_filters" = xyes || enable_clientside_filters=no
+AC_MSG_RESULT($enable_clientside_filters)
+if test "x$enable_clientside_filters" = xyes ; then
+AC_DEFINE([ENABLE_CLIENTSIDE_FILTERS], [1], [if true, enable client-side filters])
+fi
+AM_CONDITIONAL(ENABLE_CLIENTSIDE_FILTERS, [test x$enable_clientside_filters = xyes])
+
 AC_SUBST(NC_LIBS,[$NC_LIBS])
 AC_SUBST(HAS_DAP,[$enable_dap])
 AC_SUBST(HAS_DAP2,[$enable_dap])

include/hdf5internal.h

@@ -183,7 +183,7 @@ int nc4_hdf5_get_chunk_cache(int ncid, size_t *sizep, size_t *nelemsp,
 
 /* Define Filter API Function */
 int nc4_global_filter_action(int action, unsigned int id, struct NC_FILTER_OBJ_HDF5* infop);
-int NC4_hdf5_addfilter(NC_VAR_INFO_T* var, int active, unsigned int id, size_t nparams, unsigned int* params);
+int NC4_hdf5_addfilter(NC_VAR_INFO_T* var, int active, unsigned int id, size_t nparams, unsigned int* params, struct NC_FILTER_SPEC_HDF5**);
 int NC4_hdf5_remove_filter(NC_VAR_INFO_T* var, unsigned int filterid);
 
 /* Support functions for provenance info (defined in nc4hdf.c) */

include/ncconfigure.h

@@ -104,6 +104,13 @@ typedef unsigned short ushort;
 typedef unsigned int uint;
 #endif
 
+#ifndef HAVE_UINTPTR_T
+#if SIZEOF_VOIDP == 8
+typedef unsigned long uintptr_t;
+#else
+typedef unsigned int uintptr_t;
+#endif
+#endif
 
 /* Provide a fixed size alternative to off_t or off64_t */
 typedef long long fileoffset_t;

libdap4/d4curlfunctions.c

@@ -9,7 +9,7 @@
 #define MAX_REDIRECTS 20L
 
 /* Mnemonic */
-#define OPTARG void*
+#define OPTARG uintptr_t
 
 /* Condition on libcurl version */
 /* Set up an alias as needed */

libdispatch/dfile.c

@@ -1855,9 +1855,9 @@ NC_create(const char *path0, int cmode, size_t initialsz,
         for(p=(const unsigned char*)path0;*p;p++) {if(*p > ' ') break;}
 #ifdef WINPATH
         /* Need to do path conversion */
-        path = NCpathcvt(p);
+        path = NCpathcvt((const char*)p);
 #else
-        path = nulldup(p);
+        path = nulldup((const char*)p);
 #endif
     }
 
@@ -1999,8 +1999,8 @@ NC_open(const char *path0, int omode, int basepe, size_t *chunksizehintp,
 
     {
         /* Skip past any leading whitespace in path */
-        const unsigned char* p;
-        for(p=(const unsigned char*)path0;*p;p++) {if(*p > ' ') break;}
+        const char* p;
+        for(p=(const char*)path0;*p;p++) {if(*p < 0 || *p > ' ') break;}
 #ifdef WINPATH
         /* Need to do path conversion */
         path = NCpathcvt(p);

libdispatch/dfilter.c

@@ -381,6 +381,7 @@ NC4_filterfix8(unsigned char* mem, int decode)
 }
 
 
+#ifdef ENABLE_CLIENTSIDE_FILTERS
 /* Support direct user defined filters */
 
 /* Use void* to avoid having to include hdf.h*/
@@ -442,6 +443,7 @@ nc_filter_client_inq(unsigned int id, void* infop)
 #endif
     return stat;
 }
+#endif /*ENABLE_CLIENTSIDE_FILTERS*/
 
 /**
 Find the set of filters (if any) associated with a variable.

libhdf5/hdf5filter.c

@@ -23,23 +23,26 @@
 #define FILTERACTIVE 1
 
 
-/* WARNING: GLOBAL VARIABLE */
+/**************************************************/
+/* Filter registration support */
 
+#ifdef ENABLE_CLIENTSIZE_FILTERS
+
+/* WARNING: GLOBAL VARIABLE */
 /* Define list of registered filters */
 static NClist* NC4_registeredfilters = NULL; /** List<NC_FILTER_CLIENT_HDF5*> */
 
-/**************************************************/
-/* Filter registration support */
-
 static int
-filterlookup(unsigned int id)
+clientfilterlookup(unsigned int id)
 {
     int i;
     if(NC4_registeredfilters == NULL)
 	NC4_registeredfilters = nclistnew();
     for(i=0;i<nclistlength(NC4_registeredfilters);i++) {
 	NC_FILTER_CLIENT_HDF5* x = nclistget(NC4_registeredfilters,i);
-	if(x != NULL && x->id == id) return i; /* return position */
+	if(x != NULL && x->id == id) {
+	    return i; /* return position */
+	}
     }
     return -1;
 }
@@ -77,36 +80,6 @@ dupfilterinfo(NC_FILTER_CLIENT_HDF5* info)
     return NULL;
 }
 
-int
-NC4_hdf5_addfilter(NC_VAR_INFO_T* var, int active, unsigned int id, size_t nparams, unsigned int* inparams)
-{
-    int stat = NC_NOERR;
-    NC_FILTER_SPEC_HDF5* fi = NULL;
-    unsigned int* params = NULL;
-
-    if(var->filters == NULL) {
-	if((var->filters = nclistnew())==NULL) return THROW(NC_ENOMEM);
-    }
-
-    if(nparams > 0 && inparams == NULL)
-        return THROW(NC_EINVAL);
-    if(inparams != NULL) {
-        if((params = malloc(sizeof(unsigned int)*nparams)) == NULL)
-	    return THROW(NC_ENOMEM);
-        memcpy(params,inparams,sizeof(unsigned int)*nparams);
-    }
-
-    if((fi = calloc(1,sizeof(NC_FILTER_SPEC_HDF5))) == NULL)
-    	{nullfree(params); return THROW(NC_ENOMEM);}
-
-    fi->active = active;
-    fi->filterid = id;
-    fi->nparams = nparams;
-    fi->params = params;
-    nclistpush(var->filters,fi);
-    return THROW(stat);
-}
-
 int
 nc4_global_filter_action(int op, unsigned int id, NC_FILTER_OBJ_HDF5* infop)
 {
@@ -129,7 +102,7 @@ nc4_global_filter_action(int op, unsigned int id, NC_FILTER_OBJ_HDF5* infop)
         if(id != h5filterinfo->id)
 	    {stat = NC_EINVAL; goto done;}
 	/* See if this filter is already defined */
-	if((pos = filterlookup(id)) >= 0)
+	if((pos = clientfilterlookup(id)) >= 0)
 	    {stat = NC_ENAMEINUSE; goto done;} /* Already defined */
 	if((herr = H5Zregister(h5filterinfo)) < 0)
 	    {stat = NC_EFILTER; goto done;}
@@ -144,7 +117,7 @@ nc4_global_filter_action(int op, unsigned int id, NC_FILTER_OBJ_HDF5* infop)
 	if(id <= 0)
 	    {stat = NC_ENOTNC4; goto done;}
 	/* See if this filter is already defined */
-	if((pos = filterlookup(id)) < 0)
+	if((pos = clientfilterlookup(id)) < 0)
 	    {stat = NC_ENOFILTER; goto done;} /* Not defined */
 	if((herr = H5Zunregister(id)) < 0)
 	    {stat = NC_EFILTER; goto done;}
@@ -153,7 +126,7 @@ nc4_global_filter_action(int op, unsigned int id, NC_FILTER_OBJ_HDF5* infop)
     case NCFILTER_CLIENT_INQ:
 	if(infop == NULL) goto done;
         /* Look up the id in our local table */
-   	if((pos = filterlookup(id)) < 0)
+   	if((pos = clientfilterlookup(id)) < 0)
 	    {stat = NC_ENOFILTER; goto done;} /* Not defined */
         elem = (NC_FILTER_CLIENT_HDF5*)nclistget(NC4_registeredfilters,pos);
 	if(elem == NULL) {stat = NC_EINTERNAL; goto done;}
@@ -168,6 +141,61 @@ nc4_global_filter_action(int op, unsigned int id, NC_FILTER_OBJ_HDF5* infop)
     return THROW(stat);
 } 
 
+#endif /*ENABLE_CLIENTSIDE_FILTERS*/
+
+/**************************************************/
+static int
+filterlookup(NC_VAR_INFO_T* var, unsigned int id, NC_FILTER_SPEC_HDF5** fp)
+{
+    int i;
+    if(var->filters == NULL)
+	var->filters = nclistnew();
+    for(i=0;i<nclistlength(var->filters);i++) {
+	NC_FILTER_SPEC_HDF5* x = nclistget(var->filters,i);
+	if(x != NULL && x->filterid == id) {
+	    if(fp) *fp = x;
+	    return i; /* return position */
+	}
+    }
+    return -1;
+}
+
+int
+NC4_hdf5_addfilter(NC_VAR_INFO_T* var, int active, unsigned int id, size_t nparams,
+                   unsigned int* inparams, NC_FILTER_SPEC_HDF5** filtspecp)
+{
+    int stat = NC_NOERR;
+    NC_FILTER_SPEC_HDF5* fi = NULL;
+    unsigned int* params = NULL;
+    int pos;
+
+    if(var->filters == NULL) {
+	if((var->filters = nclistnew())==NULL) return THROW(NC_ENOMEM);
+    }
+
+    if(nparams > 0 && inparams == NULL)
+        return THROW(NC_EINVAL);
+    if(inparams != NULL) {
+        if((params = malloc(sizeof(unsigned int)*nparams)) == NULL)
+	    return THROW(NC_ENOMEM);
+        memcpy(params,inparams,sizeof(unsigned int)*nparams);
+    }
+    if((pos=filterlookup(var, id,&fi)) < 0) {
+	if((fi = calloc(1,sizeof(NC_FILTER_SPEC_HDF5))) == NULL)
+    	    {nullfree(params); return THROW(NC_ENOMEM);}
+    }
+    assert(fi != NULL);
+    fi->active = active;
+    fi->filterid = id;
+    fi->nparams = nparams;
+    if(fi->params) free(fi->params);
+    fi->params = params;
+    if(filtspecp) *filtspecp = fi;
+    if(pos < 0) nclistpush(var->filters,fi);
+    fi = NULL;
+    return THROW(stat);
+}
+
 /**
  * @internal Define filter settings. Called by nc_def_var_filter().
  *
@@ -238,7 +266,6 @@ NC4_filter_actions(int ncid, int varid, int op, NC_Filterobject* args)
 	params = obj->u.spec.params;
 #ifdef HAVE_H5_DEFLATE
         if(id == H5Z_FILTER_DEFLATE) {
-	    int k;
 	    int level;
             if(nparams != 1)
                 return THROW(NC_EFILTER); /* incorrect no. of parameters */
@@ -247,30 +274,23 @@ NC4_filter_actions(int ncid, int varid, int op, NC_Filterobject* args)
                 level > NC_MAX_DEFLATE_LEVEL)
                 return THROW(NC_EINVAL);
             /* If szip compression is already applied, return error. */
-	    for(k=0;k<nclistlength(var->filters);k++) {
-		NC_FILTER_SPEC_HDF5* f = nclistget(var->filters,k);
-                if (f->filterid == H5Z_FILTER_SZIP)
+	    if((filterlookup(var,H5Z_FILTER_SZIP,NULL)) >= 0)
                  return THROW(NC_EINVAL);
-	    }
         }
 #else /*!HAVE_H5_DEFLATE*/
         if(id == H5Z_FILTER_DEFLATE)
             return THROW(NC_EFILTER); /* Not allowed */
 #endif
 #ifdef HAVE_H5Z_SZIP
         if(id == H5Z_FILTER_SZIP) { /* Do error checking */
-	    int k;
             if(nparams != 2)
                 return THROW(NC_EFILTER); /* incorrect no. of parameters */
             /* Pixels per block must be an even number, < 32. */
             if (params[1] % 2 || params[1] > NC_MAX_PIXELS_PER_BLOCK)
                 return THROW(NC_EINVAL);
             /* If zlib compression is already applied, return error. */
-	    for(k=0;k<nclistlength(var->filters);k++) {
-		NC_FILTER_SPEC_HDF5* f = nclistget(var->filters,k);
-                if (f->filterid == H5Z_FILTER_DEFLATE)
+	    if((filterlookup(var,H5Z_FILTER_DEFLATE,NULL)) >= 0)
                  return THROW(NC_EINVAL);
-	    }
         }
 #else /*!HAVE_H5Z_SZIP*/
         if(id == H5Z_FILTER_SZIP)
@@ -300,8 +320,9 @@ NC4_filter_actions(int ncid, int varid, int op, NC_Filterobject* args)
                 return THROW(NC_EINVAL);
         }
 #endif
-	if((stat = NC4_hdf5_addfilter(var,!FILTERACTIVE,id,nparams,params)))
-  	    goto done;
+        if((stat = NC4_hdf5_addfilter(var,!FILTERACTIVE,id,nparams,params, NULL)))
+            goto done;
+
 #ifdef USE_PARALLEL
 #ifdef HDF5_SUPPORTS_PAR_FILTERS
         /* Switch to collective access. HDF5 requires collevtive access