h5repack oddities

[markcmiller86]

Describe the bug
I have been playing with h5repack in the context of user-defined filters (e.g. passing -f <list of objects>:<name of filter>=<filter parameters>) and have run across a number of issues which I briefly enumerate here.

Use of colon character

• When no specific objects are listed ahead of the colon character (:) in the argument to the -f option, is the colon character still needed? I think it is unclear in the documentation whether it is or not. I would recommend that it work in this circumstance either way. Then, you don't have to worry about making documentation any clearer. So, if no specific objects are listed ahead of the colon, both -f :UD=XXXXX,... and -f UD=XXXXXX,... should both work.

Handling of cd_value_count for UD= filters

• For a UD filter with no params (e.g. cd_vals), is a 0 still nonetheless needed for the cd_value_count parameter. This does seem to work either way. That is good.
• If you specify a cd_value_count of K and provide fewer than K values, this is caught as an error and h5repack does not proceed. This is also good. The reverse is not, however, true. If you specify a cd_value_count of zero but provide more values, no error is generated and, indeed, the resulting repacked datasets show those extra values as part of the filter parameters.

When filter_flag is 1 (mandatory)

• When a specific object is listed ahead of the colon character and the filter_flag is 1 indicating the filter is mandatory, then if the can_apply for the associated filter returns anything other than 1 for that object, that should be treated as an outright error by h5repack. An error message should be printed and the return value from h5repack should not be 0.
• When the requested filter ID is not succesfully loaded (e.g. after all dlopen() calls there is nothing found matching the specified filter ID) and filter_flag is 1 indicating the filter is *mandatory, that should be treated as an outright error by h5repack. An error message should be printed and the return value from h5repack should not be 0.

A stub filter I was using to test is foo.c.gz

[markcmiller86]

Related to this, I am getting a segv on HDF5-1.14.3 with a slight modification of the filter attached above.

foo.c.gz
Makefile.gz

I used the attached makefile with the command...

make HDF5_INC=/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/include
HDF5_LIB=/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/lib
SHFLAG=-dynamiclib SOEXT=so PREPATH=-Wl,-rpath, all

I then run the following command with the attached data file

mesh.h5.gz

env HDF5_DEBUG=trace HDF5_PLUGIN_PATH=`pwd`/plugin
/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/bin/h5repack -n -f UD=32333,1
mesh.h5 mesh_repack.h5

And it gives me a segmentation violation...

% env HDF5_PLUGIN_PATH=`pwd`/plugin lldb /Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/bin/h5repack
(lldb) target create "/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/bin/h5repack"
Current executable set to '/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/bin/h5repack' (x86_64).
(lldb) run -n -f UD=32333,1 mesh.h5 mesh_repack.h5
Process 98219 launched: '/Users/miller86/silo/hdf5-1.14.3/build_serial/myinstall/bin/h5repack' (x86_64)
h5repack was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 98219 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x103d31930)
    frame #0: 0x000000010015f1f4 h5repack`H5FL__reg_gc_list(head=0x0000000103d31910) at H5FL.c:497:23 [opt]
   494 	    FUNC_ENTER_PACKAGE_NOERR
   495 	
   496 	    /* For each free list being garbage collected, walk through the nodes and free them */
-> 497 	    free_list = head->list;
   498 	    while (free_list != NULL) {
   499 	        H5FL_reg_node_t *tmp; /* Temporary node pointer */
   500 	
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x103d31930)
  * frame #0: 0x000000010015f1f4 h5repack`H5FL__reg_gc_list(head=0x0000000103d31910) at H5FL.c:497:23 [opt]
    frame #1: 0x000000010015f298 h5repack`H5FL__reg_gc at H5FL.c:546:13 [opt]
    frame #2: 0x000000010015ee2f h5repack`H5FL_garbage_coll at H5FL.c:2393:9 [opt]
    frame #3: 0x000000010015edb8 h5repack`H5FL_term_package at H5FL.c:182:5 [opt]
    frame #4: 0x0000000100033c68 h5repack`H5_term_library at H5.c:456:21 [opt]
    frame #5: 0x0000000100034f0a h5repack`H5close at H5.c:1093:5 [opt]
    frame #6: 0x000000010001a876 h5repack`h5tools_close at h5tools.c:224:9 [opt]
    frame #7: 0x00000001000050fd h5repack`leave(ret=0) at h5repack_main.c:372:5 [opt]
    frame #8: 0x000000010000438e h5repack`main(argc=<unavailable>, argv=<unavailable>) at h5repack_main.c:1026:5 [opt]
    frame #9: 0x00007ff8090b3418 dyld`start + 1896

[byrnHDF]

Use of colon character

• When no specific objects are listed ahead of the colon character (:) in the argument to the -f option, is the colon character still needed? I think it is unclear in the documentation whether it is or not. I would recommend that it work in this circumstance either way. Then, you don't have to worry about making documentation any clearer. So, if no specific objects are listed ahead of the colon, both -f :UD=XXXXX,... and -f UD=XXXXXX,... should both work.

the parse for the filters has this code:

    /* Check for missing : */
    if (end_obj == -1) {
        /* apply to all objects */
        options->all_filter = 1;
        *is_glb             = 1;
        *n_objs             = 1;
    }
    else
        *n_objs = n;

The n_objs value remains set to 1, and end_obj is set to 0, which later on does the same thing as it looks for end_obj >0 to do other processing. However, the difference is the setting of the other two variables;

• options->all_filter
• is_glb /* is the filter global */

After the parse is this check, which means it will be skipped

    /* if it applies to all objects */
    if (is_glb) {
        int n;

        n = options->n_filter_g++; /* increase # of global filters */
        if (options->n_filter_g > H5_REPACK_MAX_NFILTERS) {
            error_msg("maximum number of filters exceeded for <%s>\n", str);
            free(obj_list);
            return -1;
        }

        options->filter_g[n] = filter;
    }
    else
        options_add_filter(obj_list, n_objs, filter, options->op_tbl);

the options->all_filter could have issues as there are about half-dozen checks in other parts of the code.
To make it work the same is simply to change the first check;

• if (end_obj == -1) {
  to chech for <= 0

[byrnHDF]

Handling of cd_value_count for UD= filters

• For a UD filter with no params (e.g. cd_vals), is a 0 still nonetheless needed for the cd_value_count parameter. This does seem to work either way. That is good.
• If you specify a cd_value_count of K and provide fewer than K values, this is caught as an error and h5repack does not proceed. This is also good. The reverse is not, however, true. If you specify a cd_value_count of zero but provide more values, no error is generated and, indeed, the resulting repacked datasets show those extra values as part of the filter parameters.

Wow this is a bit tricky because specifying a zero count is a special condition that defeats the error check;

            else if (strcmp(scomp, "UD") == 0) {
                /* parameters does not match count */
                if (filt->cd_nelmts != j) {
                    if (obj_list)
                        free(obj_list);
                    error_msg("incorrect number of compression parameters in <%s>\n", str);
                    exit(EXIT_FAILURE);
                }
            }

If you specify a cd_value_count of K and provide more than K values, this should be caught as an error

but the zero case loads the values before checking the count for all filters:

                filt->cd_values[j++] = (unsigned)strtoul(stype, NULL, 0);
                if (filt->cd_nelmts == 0)
                    j = 0;

I am surprised that all the values get shown because it should just overwrite the value at cd_values[0]
maybe moving the filt->cd_values[j++] line into an else of the following if check?

[byrnHDF]

I am surprised that all the values get shown because it should just overwrite the value at cd_values[0]

The reason is because the exact same line (filt->cd_values[j++] = (unsigned)strtoul(stype, NULL, 0);) is used in the UD specific block before this line. Which means that there might be a duplicate value at the end?

Looks like the previous comment assignment should be part of the else.
Adding a check for cd_nelmts = 0 and skip the cd_values assignment in the UD section should solve this.

[byrnHDF]

When filter_flag is 1 (mandatory)

• When a specific object is listed ahead of the colon character and the filter_flag is 1 indicating the filter is mandatory, then if the can_apply for the associated filter returns anything other than 1 for that object, that should be treated as an outright error by h5repack. An error message should be printed and the return value from h5repack should not be 0.
• When the requested filter ID is not succesfully loaded (e.g. after all dlopen() calls there is nothing found matching the specified filter ID) and filter_flag is 1 indicating the filter is *mandatory, that should be treated as an outright error by h5repack. An error message should be printed and the return value from h5repack should not be 0.

I think this is not a issue with h5repack but an issue for the library and the filter functions as h5repack relies on the return of the filter functions to determine there was a processing error.

However I will doublecheck that the correct values and checks are used in h5repack.

[byrnHDF]

When filter_flag is 1 (mandatory)

This is incorrect:
filter_flag: 1 is OPTIONAL or 0 is MANDATORY

[byrnHDF]

I am surprised that all the values get shown because it should just overwrite the value at cd_values[0]

The reason is because the exact same line (filt->cd_values[j++] = (unsigned)strtoul(stype, NULL, 0);) is used in the UD specific block before this line. Which means that there might be a duplicate value at the end?

Looks like the previous comment assignment should be part of the else. Adding a check for cd_nelmts = 0 and skip the cd_values assignment in the UD section should solve this.

This was wrong because the line for the last value is needed as there is one more value to be added, but if cd_elmts is zero why add a value?

Only UD uses cd_nelmts variable.