Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Another round of Coverity cleanups, (minor) speedups, bug-fixes and test updates. #14

Closed
wants to merge 6 commits into from

Conversation

qkoziol
Copy link
Contributor

@qkoziol qkoziol commented Dec 29, 2013

Corrected "BAIL" macros to avoid infinite loop when logging is disabled and an
error occurs after an "exit:" label.

Corrected a dozen Coverity errors (mainly allocation issues, along with a few
other things):
711711, 711802, 711803, 711905, 970825, 996123, 996124, 1025787,
1047274, 1130013, 1130014, 1139538

Refactored internal fill-value code to correctly handle string types, and
especially to allow NULL pointers and null strings (ie. "") to be
distinguished. The code now avoids partially aliasing the two together
(which only happened on the 'write' side of things and wasn't reflected on
the 'read' side, adding to the previous confusion).

Probably still weak on handling fill-values of variable-length and compound
datatypes.

Refactored the recursive metadata reads a bit more, to process HDF5 named
datatypes and datasets immediately, avoiding chewing up memory for those
types of objects, etc.

Finished uncommenting and updating the nc_test4/tst_fills2.c code (as I'm
proceeding alphabetically through the nc_test4 code files).

…ed and an

    error occurs after an "exit:" label.

Corrected a dozen Coverity errors (mainly allocation issues, along with a few
    other things):
        711711, 711802, 711803, 711905, 970825, 996123, 996124, 1025787,
        1047274, 1130013, 1130014, 1139538

Refactored internal fill-value code to correctly handle string types, and
    especially to allow NULL pointers and null strings (ie. "") to be
    distinguished.  The code now avoids partially aliasing the two together
    (which only happened on the 'write' side of things and wasn't reflected on
    the 'read' side, adding to the previous confusion).

    Probably still weak on handling fill-values of variable-length and compound
    datatypes.

Refactored the recursive metadata reads a bit more, to process HDF5 named
    datatypes and datasets immediately, avoiding chewing up memory for those
    types of objects, etc.

Finished uncommenting and updating the nc_test4/tst_fills2.c code (as I'm
    proceeding alphabetically through the nc_test4 code files).
@WardF
Copy link
Member

WardF commented Dec 30, 2013

Seeing a failure in tst_strings.c at line 193 with this pull request:

 Program received signal EXC_BAD_ACCESS, Could not access memory.
 Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000

I've poked around a bit, but since tst_strings.c is unchanged from the trunk, the issue must be related to a change elsewhere; I'll continue to poke around, but the more eyes on the problem the better :).

For completeness sake, I've tested this on an OSX and 32-bit Linux machine. NetCDF was built as follows:

$ mkdir build && cd build && cmake .. -DENABLE_COVERAGE_TESTS=ON -DENABLE_EXTRA_TESTS=ON && time make Experimental

Information regarding this failure may be found on the netCDF CDash Dashboard: http://my.cdash.org/testSummary.php?project=644&name=nc_test4_tst_strings&date=2013-12-30

I'm not sure about the proper workflow; whether to close this pull request, or if it can be updated in-place.

-Ward

@qkoziol
Copy link
Contributor Author

qkoziol commented Dec 30, 2013

Hi Ward,
It's probably related to my changes to how string fill-values are handled. I'll try to take a look into it tonight. (I think the pull request can be updated in place)

Quincey

On Dec 30, 2013, at 4:57 PM, Ward Fisher [email protected] wrote:

Seeing a failure in tst_strings.c at line 193 with this pull request:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
I've poked around a bit, but since tst_strings.c is unchanged from the trunk, the issue must be related to a change elsewhere; the more eyes on the problem the better :).

For completeness sake, I've tested this on an OSX and 32-bit Linux machine. NetCDF was built as follows:

$ mkdir build && cd build && cmake .. -DENABLE_COVERAGE_TESTS=ON -DENABLE_EXTRA_TESTS=ON && time make Experimental
Information regarding this failure may be found on the netCDF CDash Dashboard: http://my.cdash.org/testSummary.php?project=644&name=nc_test4_tst_strings&date=2013-12-30

I'm not sure about the proper workflow; whether to close this pull request, or if it can be updated in-place.

-Ward


Reply to this email directly or view it on GitHub.

@qkoziol
Copy link
Contributor Author

qkoziol commented Jan 2, 2014

Hi Ward,
I've worked on this over the last few days and it's being sticky, so it might take another day or two... :-/

    Quincey

On Dec 30, 2013, at 4:57 PM, Ward Fisher [email protected] wrote:

Seeing a failure in tst_strings.c at line 193 with this pull request:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
I've poked around a bit, but since tst_strings.c is unchanged from the trunk, the issue must be related to a change elsewhere; the more eyes on the problem the better :).

For completeness sake, I've tested this on an OSX and 32-bit Linux machine. NetCDF was built as follows:

$ mkdir build && cd build && cmake .. -DENABLE_COVERAGE_TESTS=ON -DENABLE_EXTRA_TESTS=ON && time make Experimental
Information regarding this failure may be found on the netCDF CDash Dashboard: http://my.cdash.org/testSummary.php?project=644&name=nc_test4_tst_strings&date=2013-12-30

I'm not sure about the proper workflow; whether to close this pull request, or if it can be updated in-place.

-Ward


Reply to this email directly or view it on GitHub.

@WardF
Copy link
Member

WardF commented Jan 3, 2014

Hi Quincey,

No problem. If you are interested, our CDash dashboard is publicly available:

http://my.cdash.org/index.php?project=netcdf-c

When configuring/building with cmake, you can use the experimental make target to submit build/test information to this dashboard. Are you registered on cdash.org? If so, I'll add you to the project so that you can dive in and see detailed information. It occurs to me now that you might not be able to see all of the diagnostic info without an account.

If you aren't registered, I'll set up an account for you using the email address that you're using for the rest of the contract work.

My workflow typically goes like this, just for reference:

$ cd netcdf-c && mkdir build && cd build
$ cmake .. -DENABLE_COVERAGE_TESTS=ON -DENABLE_EXTRA_TESTS=ON && make Experimental
Check cdash website to see if there were any failures or changes.

The nice thing with this type of testing is that tests aren't just evaluated on whether they passed or failed, but if the time it took to run was more than 2 standard deviations from the 'normal' test time. If so, it flags a timing failure to let you know that the test passed, but ran much more slowly than usual.

@qkoziol
Copy link
Contributor Author

qkoziol commented Feb 12, 2014

I've added some more commits to this (I think). Are they added to my previous changes, or do I need to use a separate pull request?

Here's my changelog for the most recent set of commits:

h5_test/tst_h_vars2.c:
Corrected usage of H5Lget_name_by_idx()

include/nc4internal.h:
Added new [internal] boolean type, nc_bool_t, so that it's clear when a variable is
supposed to be TRUE/FALSE and not a generic integer.
Switched fields in various structs to use new nc_bool_t type.
Eliminated some unused fields in structs.
Added more comments explaining purpose of fields in structs.
Big refactor on how types are used for attributes, variables and committed types,
clarifying and categorizing fields in structs, also eliminating duplicated type
information between variables and the type it uses.
Made type structure ref. counted, so it can be shared properly by committed datatypes
and variables that use it.
Added a "class" categorization for types, so that it's easier to detect integer/float/
string/etc. types (rather than checking against the individual type values).
Made names of fields used for the same purpose uniform between structs.
Eliminated some unused prototypes.
Added 'const' to some pointer parameters in various routines.

include/nc_logging.h:
Clean up indentation.
Re-organized macros to eliminate duplication.

include/ncdispatch.h:
Switched extern variables to arrays instead of pointers, for better range-checking.

libdap2/common34.c:
Cleaned up code for memory errors.

libdap2/ncdap3.c:
Eliminated unused code.

libdispatch/dfile.c:
Switched global variables to arrays instead of pointers, for better range-checking
and less dynamic memory usage.

libsrc/ncx.c:
Cleaned up some compiler/Coverity warnings.

libsrc/ncx.m4:
Cleaned up some compiler/Coverity warnings.

libsrc/putget.c:
Cleaned up some compiler/Coverity warnings.

libsrc4/Makefile.am:
Alphabetized source files, so builds are more predictable.

libsrc4/nc4attr.c:
Refactoring, based on changes to internal structures and new nc_bool_t type.
Make coding style more uniform with rest of source.
Handle string datatypes correctly, particularly for fill value attributes.
Switched a few routines to be staticly declared, since they aren't used outside this
module.

libsrc4/nc4dim.c:
Refactoring, for new nc_bool_t type.

libsrc4/nc4file.c:
Removed some fields from type used when iterating over file (see below).
Moved some locally static declared arrays (within a function) to be globally static,
and eliminate duplication of these arrays.
Refactoring, based on changes to internal structures and new nc_bool_t type.
Switched variables that check HDF5 datatype class from hid_t -> H5T_class_t (per
HDF5's function declarations).
Cleaned up type information for variables read in from the file.
Cleaned up parsing of committed datatypes read in from the file.
Switch more code to using 'BAIL' macros, instead of return'ing from the middle of a
routine (so that cleanups can occur on error detection).
Simplified iteration of objects in the file when it's opened, reducing the objects
that are tracked and memory used. (Only the datasets (i.e. variables) need to
be deferred now, so that they have access to all the datatypes in the file)
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/nc4hdf.c:
Refactoring, based on changes to internal structures and new nc_bool_t type.
Corrected fill-value routines, to handle strings properly.
Restructured code for retrieving HDF5 datatype from netCDF-4 type, to properly
handle strings.
General code cleanups and dead variable removal.
Switch more code to using 'BAIL' macros, instead of return'ing from the middle of a
routine (so that cleanups can occur on error detection).
Properly clean up HDF5 datatypes for array fields in compound datatypes.
Added nc4_get_typeclass() routine, to map netCDF-4 types to new type class.

libsrc4/nc4internal.c:
Refactoring, based on changes to internal structures and new nc_bool_t type.
Rearranged "find type" routines, to better match usage.
Refactored nc4_type_list_add() to initialize common variables.
Added nc4_type_free() routine, to free memory for type, eliminating duplicated code
in library.
Release fill-value information for variables correctly.

libsrc4/nc4type.c:
Refactoring, based on changes to internal structures and new nc_bool_t type.

libsrc4/nc4var.c:
Refactoring, based on changes to internal structures and new nc_bool_t type.
Switch more code to using 'BAIL' macros, instead of return'ing from the middle of a
routine (so that cleanups can occur on error detection).
Corrected fill-value handling, particularly for strings.
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/ncfunc.c:
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

nc_test4/tst_fills2.c:
Uncomment string testing for fill values, expanding it somewhat also.

nc_test4/tst_strings.c:
Corrected spelling of U.S. Presidents' names. :-)
Fixed handling of NULL pointers for strings.

ncdump/cdl4/ref_typescope.cdl:
Corrected comment

ncdump/nccopy.c:
Cleaned up some compiler/Coverity warnings.

ncdump/ncdump.c:
Cleaned up some compiler/Coverity warnings.

ncdump/utils.c:
Added freeidlist() routine, to assist with compiler/Coverity motivated cleanups.

ncdump/utils.h:
Added prototype for freeidlist() routine.

ncgen/ncgen.l:
Cleaned up some compiler/Coverity warnings.

ncgen/ncgen.y:
Cleaned up some compiler/Coverity warnings.

ncgen3/c0.cdl:
Uncommented test data that was failing on T3E (which no longer exist).

oc2/ocutil.c:
Cleaned up some compiler/Coverity warnings.

@WardF
Copy link
Member

WardF commented Feb 12, 2014

On 2/12/14, 7:53 AM, Quincey Koziol wrote:

I've added some more commits to this (I think). Are they added to my
previous changes, or do I need to use a separate pull request?

Hi Quincey,

I wasn't sure of this, but asking the git guru's at lunch, it seems like
the SOP for pull requests that are found to have issues is to close them
and then issue new pull requests; new commits are not added to existing
changes/pull requests. So perhaps we should close the existing pull
requests and issue a new one? We can talk more about it at our
conference call.

-Ward

Here's my changelog for the most recent set of commits:

h5_test/tst_h_vars2.c:
Corrected usage of H5Lget_name_by_idx()

include/nc4internal.h:
Added new [internal] boolean type, nc_bool_t, so that it's clear when
a variable is
supposed to be TRUE/FALSE and not a generic integer.
Switched fields in various structs to use new nc_bool_t type.
Eliminated some unused fields in structs.
Added more comments explaining purpose of fields in structs.
Big refactor on how types are used for attributes, variables and
committed types,
clarifying and categorizing fields in structs, also eliminating
duplicated type
information between variables and the type it uses.
Made type structure ref. counted, so it can be shared properly by
committed datatypes
and variables that use it.
Added a "class" categorization for types, so that it's easier to
detect integer/float/
string/etc. types (rather than checking against the individual type
values).
Made names of fields used for the same purpose uniform between structs.
Eliminated some unused prototypes.
Added 'const' to some pointer parameters in various routines.

include/nc_logging.h:
Clean up indentation.
Re-organized macros to eliminate duplication.

include/ncdispatch.h:
Switched extern variables to arrays instead of pointers, for better
range-checking.

libdap2/common34.c:
Cleaned up code for memory errors.

libdap2/ncdap3.c:
Eliminated unused code.

libdispatch/dfile.c:
Switched global variables to arrays instead of pointers, for better
range-checking
and less dynamic memory usage.

libsrc/ncx.c:
Cleaned up some compiler/Coverity warnings.

libsrc/ncx.m4:
Cleaned up some compiler/Coverity warnings.

libsrc/putget.c:
Cleaned up some compiler/Coverity warnings.

libsrc4/Makefile.am:
Alphabetized source files, so builds are more predictable.

libsrc4/nc4attr.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Make coding style more uniform with rest of source.
Handle string datatypes correctly, particularly for fill value attributes.
Switched a few routines to be staticly declared, since they aren't
used outside this
module.

libsrc4/nc4dim.c:
Refactoring, for new nc_bool_t type.

libsrc4/nc4file.c:
Removed some fields from type used when iterating over file (see below).
Moved some locally static declared arrays (within a function) to be
globally static,
and eliminate duplication of these arrays.
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Switched variables that check HDF5 datatype class from hid_t ->
H5T_class_t (per
HDF5's function declarations).
Cleaned up type information for variables read in from the file.
Cleaned up parsing of committed datatypes read in from the file.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Simplified iteration of objects in the file when it's opened, reducing
the objects
that are tracked and memory used. (Only the datasets (i.e. variables)
need to
be deferred now, so that they have access to all the datatypes in the
file)
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/nc4hdf.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Corrected fill-value routines, to handle strings properly.
Restructured code for retrieving HDF5 datatype from netCDF-4 type, to
properly
handle strings.
General code cleanups and dead variable removal.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Properly clean up HDF5 datatypes for array fields in compound datatypes.
Added nc4_get_typeclass() routine, to map netCDF-4 types to new type
class.

libsrc4/nc4internal.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Rearranged "find type" routines, to better match usage.
Refactored nc4_type_list_add() to initialize common variables.
Added nc4_type_free() routine, to free memory for type, eliminating
duplicated code
in library.
Release fill-value information for variables correctly.

libsrc4/nc4type.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.

libsrc4/nc4var.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Corrected fill-value handling, particularly for strings.
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/ncfunc.c:
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

nc_test4/tst_fills2.c:
Uncomment string testing for fill values, expanding it somewhat also.

nc_test4/tst_strings.c:
Corrected spelling of U.S. Presidents' names. :-)
Fixed handling of NULL pointers for strings.

ncdump/cdl4/ref_typescope.cdl:
Corrected comment

ncdump/nccopy.c:
Cleaned up some compiler/Coverity warnings.

ncdump/ncdump.c:
Cleaned up some compiler/Coverity warnings.

ncdump/utils.c:
Added freeidlist() routine, to assist with compiler/Coverity motivated
cleanups.

ncdump/utils.h:
Added prototype for freeidlist() routine.

ncgen/ncgen.l:
Cleaned up some compiler/Coverity warnings.

ncgen/ncgen.y:
Cleaned up some compiler/Coverity warnings.

ncgen3/c0.cdl:
Uncommented test data that was failing on T3E (which no longer exist).

oc2/ocutil.c:
Cleaned up some compiler/Coverity warnings.


Reply to this email directly or view it on GitHub
#14 (comment).

@qkoziol
Copy link
Contributor Author

qkoziol commented Feb 12, 2014

Hi Ward,

On Feb 12, 2014, at 1:28 PM, Ward Fisher [email protected] wrote:

On 2/12/14, 7:53 AM, Quincey Koziol wrote:

I've added some more commits to this (I think). Are they added to my
previous changes, or do I need to use a separate pull request?

Hi Quincey,

I wasn't sure of this, but asking the git guru's at lunch, it seems like
the SOP for pull requests that are found to have issues is to close them
and then issue new pull requests; new commits are not added to existing
changes/pull requests. So perhaps we should close the existing pull
requests and issue a new one? We can talk more about it at our
conference call.

Issuing a new pull request isn't any difficulty, but yes, let's talk about it in 20 minutes.  :-)

    Quincey

-Ward

Here's my changelog for the most recent set of commits:

h5_test/tst_h_vars2.c:
Corrected usage of H5Lget_name_by_idx()

include/nc4internal.h:
Added new [internal] boolean type, nc_bool_t, so that it's clear when
a variable is
supposed to be TRUE/FALSE and not a generic integer.
Switched fields in various structs to use new nc_bool_t type.
Eliminated some unused fields in structs.
Added more comments explaining purpose of fields in structs.
Big refactor on how types are used for attributes, variables and
committed types,
clarifying and categorizing fields in structs, also eliminating
duplicated type
information between variables and the type it uses.
Made type structure ref. counted, so it can be shared properly by
committed datatypes
and variables that use it.
Added a "class" categorization for types, so that it's easier to
detect integer/float/
string/etc. types (rather than checking against the individual type
values).
Made names of fields used for the same purpose uniform between structs.
Eliminated some unused prototypes.
Added 'const' to some pointer parameters in various routines.

include/nc_logging.h:
Clean up indentation.
Re-organized macros to eliminate duplication.

include/ncdispatch.h:
Switched extern variables to arrays instead of pointers, for better
range-checking.

libdap2/common34.c:
Cleaned up code for memory errors.

libdap2/ncdap3.c:
Eliminated unused code.

libdispatch/dfile.c:
Switched global variables to arrays instead of pointers, for better
range-checking
and less dynamic memory usage.

libsrc/ncx.c:
Cleaned up some compiler/Coverity warnings.

libsrc/ncx.m4:
Cleaned up some compiler/Coverity warnings.

libsrc/putget.c:
Cleaned up some compiler/Coverity warnings.

libsrc4/Makefile.am:
Alphabetized source files, so builds are more predictable.

libsrc4/nc4attr.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Make coding style more uniform with rest of source.
Handle string datatypes correctly, particularly for fill value attributes.
Switched a few routines to be staticly declared, since they aren't
used outside this
module.

libsrc4/nc4dim.c:
Refactoring, for new nc_bool_t type.

libsrc4/nc4file.c:
Removed some fields from type used when iterating over file (see below).
Moved some locally static declared arrays (within a function) to be
globally static,
and eliminate duplication of these arrays.
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Switched variables that check HDF5 datatype class from hid_t ->
H5T_class_t (per
HDF5's function declarations).
Cleaned up type information for variables read in from the file.
Cleaned up parsing of committed datatypes read in from the file.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Simplified iteration of objects in the file when it's opened, reducing
the objects
that are tracked and memory used. (Only the datasets (i.e. variables)
need to
be deferred now, so that they have access to all the datatypes in the
file)
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/nc4hdf.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Corrected fill-value routines, to handle strings properly.
Restructured code for retrieving HDF5 datatype from netCDF-4 type, to
properly
handle strings.
General code cleanups and dead variable removal.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Properly clean up HDF5 datatypes for array fields in compound datatypes.
Added nc4_get_typeclass() routine, to map netCDF-4 types to new type
class.

libsrc4/nc4internal.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Rearranged "find type" routines, to better match usage.
Refactored nc4_type_list_add() to initialize common variables.
Added nc4_type_free() routine, to free memory for type, eliminating
duplicated code
in library.
Release fill-value information for variables correctly.

libsrc4/nc4type.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.

libsrc4/nc4var.c:
Refactoring, based on changes to internal structures and new nc_bool_t
type.
Switch more code to using 'BAIL' macros, instead of return'ing from
the middle of a
routine (so that cleanups can occur on error detection).
Corrected fill-value handling, particularly for strings.
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

libsrc4/ncfunc.c:
Cleaned up HDF4 code, to move more under the "#ifdef HDF4".

nc_test4/tst_fills2.c:
Uncomment string testing for fill values, expanding it somewhat also.

nc_test4/tst_strings.c:
Corrected spelling of U.S. Presidents' names. :-)
Fixed handling of NULL pointers for strings.

ncdump/cdl4/ref_typescope.cdl:
Corrected comment

ncdump/nccopy.c:
Cleaned up some compiler/Coverity warnings.

ncdump/ncdump.c:
Cleaned up some compiler/Coverity warnings.

ncdump/utils.c:
Added freeidlist() routine, to assist with compiler/Coverity motivated
cleanups.

ncdump/utils.h:
Added prototype for freeidlist() routine.

ncgen/ncgen.l:
Cleaned up some compiler/Coverity warnings.

ncgen/ncgen.y:
Cleaned up some compiler/Coverity warnings.

ncgen3/c0.cdl:
Uncommented test data that was failing on T3E (which no longer exist).

oc2/ocutil.c:
Cleaned up some compiler/Coverity warnings.


Reply to this email directly or view it on GitHub
#14 (comment).


Reply to this email directly or view it on GitHub.

@qkoziol qkoziol closed this Feb 12, 2014
edhartnett added a commit to NetCDF-World-Domination-Council/netcdf-c that referenced this pull request Feb 4, 2018
…h_dispatch

move hdf4 to its own dispatch layer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants