Add tests for H5R get name APIs #4657

bmribler · 2024-07-17T04:39:08Z

This PR adds functionality tests for the following APIs:
H5Rget_file_name
H5Rget_obj_name
H5Rget_attr_name

It also removes a "+1" when returning a name length in H5R__get_attr_name() in the Fortran wrapper. This extra "+1" gave an incorrect value for the length of the referenced object's attribute name. In addition, the PR provides some minor improvement in readability and maintainability.

(Fixed GH-4447)

Added functionality tests for the following APIs: H5Rget_file_name H5Rget_obj_name H5Rget_attr_name Also removed "+1" when returning a name length in H5R__get_attr_name(). The exter "+1" gave an incorrect value for the length of the referenced object's attribute name. Fixed HDFGroupGH-4447

…into fix_GH-4447

The C API H5Rget_attr_name incorrectly added 1 to the length of the referenced object's attribute name, so the Fortran API h5rget_attr_name_f removed 1 from the returned value to accommodate the incorrectness. This PR fixes H5Rget_attr_name so this workaround in h5rget_attr_name_f is no longer needed.

jhendersonHDF · 2024-07-29T15:31:34Z

Note that H5R__get_file_name also appears to add a +1. We should add a test that compares the length returned by H5Fget_name vs. H5Rget_file_name to make sure they're consistent, as well as adding a test for H5Aget_name vs. H5Rget_attr_name and H5Iget_name vs. H5Rget_obj_name.

brtnfld · 2024-07-29T16:25:02Z

fortran/src/H5Rff.F90

@@ -1180,8 +1180,6 @@ END FUNCTION H5Rget_attr_name
       c_name(1:1)(1:1) = C_NULL_CHAR
       name_len = H5Rget_attr_name(ref_ptr, c_name, 1_SIZE_T)
       IF(name_len.LT.0_SIZE_T) hdferr = H5I_INVALID_HID_F
-       ! Don't include the NULL term in the size
-       name_len = name_len - 1


To clarify, H5Rget_attr_name returned length does not include the NULL character now with this PR? If that is the case, the doxygen doc needs to be updated as it says it does include NULL.

bmribler · 2024-07-29T18:30:13Z

Note that H5R__get_file_name also appears to add a +1. We should add a test that compares the length returned by H5Fget_name vs. H5Rget_file_name to make sure they're consistent, as well as adding a test for H5Aget_name vs. H5Rget_attr_name and H5Iget_name vs. H5Rget_obj_name.

Hmm, the documentation says that the length returned does not include the null terminator for H5Rget_file_name.

I accidentally deleted the test results so I don't remember exact detail, but I remember clearly the length returned had exact number of characters in the name, so no null terminator. That was why the Fortran test failed in that PR before I committed the fix.

jhendersonHDF · 2024-07-29T19:06:32Z

Note that H5R__get_file_name also appears to add a +1. We should add a test that compares the length returned by H5Fget_name vs. H5Rget_file_name to make sure they're consistent, as well as adding a test for H5Aget_name vs. H5Rget_attr_name and H5Iget_name vs. H5Rget_obj_name.

Hmm, the documentation says that the length returned does not include the null terminator for H5Rget_file_name.

I accidentally deleted the test results so I don't remember exact detail, but I remember clearly the length returned had exact number of characters in the name, so no null terminator. That was why the Fortran test failed in that PR before I committed the fix.

It looks like the case where H5R__get_file_name is called is with unopened external references so the test case probably wasn't going down that path. We would need a more specific test case to hit the issue.

bmribler · 2024-07-29T19:14:30Z

Note that H5R__get_file_name also appears to add a +1. We should add a test that compares the length returned by H5Fget_name vs. H5Rget_file_name to make sure they're consistent, as well as adding a test for H5Aget_name vs. H5Rget_attr_name and H5Iget_name vs. H5Rget_obj_name.

Hmm, the documentation says that the length returned does not include the null terminator for H5Rget_file_name.

I accidentally deleted the test results so I don't remember exact detail, but I remember clearly the length returned had exact number of characters in the name, so no null terminator. That was why the Fortran test failed in that PR before I committed the fix.

It looks like the case where H5R__get_file_name is called is with unopened external references so the test case probably wasn't going down that path. We would need a more specific test case to hit the issue.

OK

…H-4447

brtnfld · 2024-07-29T21:29:07Z

I accidentally deleted the test results so I don't remember exact detail, but I remember clearly the length returned had exact number of characters in the name, so no null terminator. That was why the Fortran test failed in that PR before I committed the fix.

Are you talking about H5Rget_file_name? Otherwise, originally, the length of H5Rget_attr_name did include the null terminator, which Fortran was subtracting from the length returned by the function.

bmribler · 2024-07-30T07:12:18Z

I accidentally deleted the test results so I don't remember exact detail, but I remember clearly the length returned had exact number of characters in the name, so no null terminator. That was why the Fortran test failed in that PR before I committed the fix.

Are you talking about H5Rget_file_name? Otherwise, originally, the length of H5Rget_attr_name did include the null terminator, which Fortran was subtracting from the length returned by the function.

Yes, I found that H5Rget_attr_name included null terminator while other get name functions I checked didn't, so I removed +1 there to be consistent with the other getname functions. But after I removed +1 in H5Rget_attr_name, the Fortran test failed, hence, I made the change there.

So, what is the decision about the null terminator for getname functions? Should I get a tally of the current way to help us decide? I will update the documentation where necessary.

brtnfld · 2024-07-30T15:34:57Z

I think this PR addresses the immediate issue; the get_name issue can be looked into later.

bmribler · 2024-07-30T17:22:18Z

I think this PR addresses the immediate issue; the get_name issue can be looked into later.

Ok, good, thanks! I'll make sure that the documentation will say no null terminator for these particular functions and add a RELEASE.txt entry.

BTW, the comment "I'm just wondering if such API changes should be made in a minor release." has not been discussed... Should this not go in 1.14.6?

lrknox · 2024-08-07T18:48:33Z

I agree; consistency would be good. I'm just wondering if such API changes should be made in a minor release.

@brtnfld, were you referring to API changes in this PR, or to other potential changes?

lrknox · 2024-08-07T18:51:26Z

I think this PR addresses the immediate issue; the get_name issue can be looked into later.

Ok, good, thanks! I'll make sure that the documentation will say no null terminator for these particular functions and add a RELEASE.txt entry.

BTW, the comment "I'm just wondering if such API changes should be made in a minor release." has not been discussed... Should this not go in 1.14.6?

It looks like Scot was ok with the PR, though he didn't officially approve it, and noone answered this question. The PR has 3 approvals; if noone is requests changes by tomorrow I think we should merge this.

brtnfld · 2024-08-07T20:10:03Z

I think @bmribler is going to update the Doxygen documentationwill update the Doxygen documentation and release notes in the PR before it gets merged.

bmribler · 2024-08-07T20:13:45Z

I think @bmribler is going to update the Doxygen documentationwill update the Doxygen documentation and release notes in the PR before it gets merged.

Yes, I'll do it today or tomorrow.

bmribler · 2024-08-09T04:59:01Z

@jhendersonHDF @brtnfld @qkoziol
Please look at issue #4704 and let me know if we still want to remove the null terminator from H5Rget_attr_name in this PR. Or should I check the code to verify the doc info in #4704 first, before deciding about H5Rget_attr_name here?

I think removing the null terminator in this PR is a good change and the decision seemed to have been made, but now I'm a little concerned about making the inconsistency issue worse later, so I'm bringing this up again to be sure that we have a consensus. Thanks!

brtnfld · 2024-08-09T18:47:36Z

I think the consensus going forward was to never include the null terminator, so this PR should be consistent with future changes.

bmribler · 2024-08-09T19:15:59Z

I think the consensus going forward was to never include the null terminator, so this PR should be consistent with future changes.

Thanks! I'll go ahead and wrap this up.

qkoziol · 2024-08-10T17:05:26Z

I think the consensus going forward was to never include the null terminator, so this PR should be consistent with future changes.

Agree. These routines should behave like strlen().

brtnfld · 2024-08-12T14:26:34Z

release_docs/RELEASE.txt

+      without the null terminator.  H5Rget_attr_name now behaves consistently
+      with the other two APIs.  Going forward, all the get character string
+      APIs in HDF5 will be modified/written in this manner, regarding the
+      length of a character string.


remove the comma before "regarding".

Added functionality tests for the following APIs: H5Rget_file_name H5Rget_obj_name H5Rget_attr_name Also removed "+1" when returning a name length in H5R__get_attr_name(). The exter "+1" gave an incorrect value for the length of the referenced object's attribute name. Fixed HDFGroupGH-4447 * Fix Fortran test The C API H5Rget_attr_name incorrectly added 1 to the length of the referenced object's attribute name, so the Fortran API h5rget_attr_name_f removed 1 from the returned value to accommodate the incorrectness. This PR fixes H5Rget_attr_name so this workaround in h5rget_attr_name_f is no longer needed. * Add test H5Aget_name against H5Rget_attr_name

* Warning fix (#4682) * warning fix * warning fix * CMake link line needs to use new HDF5_ENABLE_THREADS (#4685) * Correct the properties for using THREADS library (#4690) * Bump the github-actions group with 5 updates (#4688) Bumps the github-actions group with 5 updates: | Package | From | To | | --- | --- | --- | | [actions/download-artifact](https://github.com/actions/download-artifact) | `4.1.7` | `4.1.8` | | [DoozyX/clang-format-lint-action](https://github.com/doozyx/clang-format-lint-action) | `0.13` | `0.17` | | [softprops/action-gh-release](https://github.com/softprops/action-gh-release) | `2.0.6` | `2.0.8` | | [ossf/scorecard-action](https://github.com/ossf/scorecard-action) | `2.3.3` | `2.4.0` | | [github/codeql-action](https://github.com/github/codeql-action) | `3.25.11` | `3.25.15` | Updates `actions/download-artifact` from 4.1.7 to 4.1.8 - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@65a9edc...fa0a91b) Updates `DoozyX/clang-format-lint-action` from 0.13 to 0.17 - [Release notes](https://github.com/doozyx/clang-format-lint-action/releases) - [Commits](DoozyX/clang-format-lint-action@v0.13...v0.17) Updates `softprops/action-gh-release` from 2.0.6 to 2.0.8 - [Release notes](https://github.com/softprops/action-gh-release/releases) - [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md) - [Commits](softprops/action-gh-release@a74c6b7...c062e08) Updates `ossf/scorecard-action` from 2.3.3 to 2.4.0 - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](ossf/scorecard-action@dc50aa9...62b2cac) Updates `github/codeql-action` from 3.25.11 to 3.25.15 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@b611370...afb54ba) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: DoozyX/clang-format-lint-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: softprops/action-gh-release dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: ossf/scorecard-action dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... * Fix segfault when closing datatype during failure in H5Topen2 (#4683) * Rework Dynamic Analysis and sanitize testing (#4681) * Ignore predetermined failing test and check pointer before use * Rework Analysis process * Remove another H5E_BEGIN/END_TRY within the library (#4675) * Update logic for (deprecated) H5Gget_objinfo() call to eliminate H5E_BEGIN_TRY * Handle case for '.' at the end of a path * Drop H5E_BEGIN/END_TRY and just check the error return from H5I_clear_types() (#4694) Original case that the change in commit 2dc738a no longer applies. * Add check of returned value from API calls. (#4702) These were found while investigating GH-4672, but they were not related to GH-4672. * Add mac dmg binary and remove old macos-13 workflows (#4699) * Add Windows SHLWAPI lib to public interface (#4701) * Use local variable in btree2 and print value (#4679) * Correct logic * Technically, level 1 Express could skip tests * Add windows signing (#4703) * Add tests for H5R get name APIs (#4657) Added functionality tests for the following APIs: H5Rget_file_name H5Rget_obj_name H5Rget_attr_name Also removed "+1" when returning a name length in H5R__get_attr_name(). The exter "+1" gave an incorrect value for the length of the referenced object's attribute name. Fixed GH-4447 * Fix Fortran test The C API H5Rget_attr_name incorrectly added 1 to the length of the referenced object's attribute name, so the Fortran API h5rget_attr_name_f removed 1 from the returned value to accommodate the incorrectness. This PR fixes H5Rget_attr_name so this workaround in h5rget_attr_name_f is no longer needed. * Add test H5Aget_name against H5Rget_attr_name * Replace Visual Studio ???? with 2022 in MSI README file (#4709) * Change logic for checking secrets exists (#4711) * Change osx refs to macos (#4707) * Replace alias \Code with \TText (#4714) Fixed GH-2151 * Correct signing names and variables (#4713) * Add secrets to release workflow (#4719) * Add missing blosc2 info (#4717) * Fix error return types in H5Rdeprec.c (#4722) Copy-pasted code from elsewhere used FAIL instead of H5G_UNKNOWN and H5I_INVALID_HID. * Fix the release reference name (#4721) * Test creating unseekable file (#4720) * Cleanup up tests (#4724) * Add arch name to dmg file name (#4732) The binaries in snapshot dmg file do not work on x86_64. * Fix snapshot CI failure by adding arch name to dmg file (#4734) See also #4732. * Fix incorrect VOL vs. non-VOL calls partially (#4733) * Fix incorrect VOL vs. non-VOL calls H5Lget_info2() called H5I_object() instead of H5VL_vol_object() crashed user application. This is a wide-spread issue (GH-4730) but this PR only addresses GH-4705. * Remove an incorrect change * Fix segfault in ROS3 credential parsing (#4736) * Fix segfault in s3 credential parsing * Fix AWS cred parsing when >1 profile provided * Revert gh-pages action hash to fix daily build (#4735) * Revert gh-pages action hash to fix daily build See also #4734 * Revert gh-pages action hash to fix daily build * Eliminate another use of H5E_clear_stack() within the library (#4726) * Remove call to H5E_clear_stack() Also clean up a bunch of error macros and the return value from H5B_valid()

bmribler and others added 2 commits July 16, 2024 23:35

Committing clang-format changes

0756fc0

bmribler requested review from lrknox, derobins, byrnHDF, fortnern, jhendersonHDF, qkoziol, vchoi-hdfgroup, glennsong09, mattjala and brtnfld as code owners July 17, 2024 04:39

bmribler closed this Jul 17, 2024

bmribler added 3 commits July 17, 2024 00:54

Fix typo

f5690de

Merge branch 'fix_GH-4447' of https://github.com/bmribler/hdf5_bmr23 …

60e3698

…into fix_GH-4447

Fix typo

49404be

bmribler reopened this Jul 17, 2024

bmribler marked this pull request as draft July 17, 2024 06:03

bmribler marked this pull request as ready for review July 17, 2024 07:45

bmribler requested a review from epourmal as a code owner July 17, 2024 07:45

byrnHDF approved these changes Jul 17, 2024

View reviewed changes

byrnHDF added Priority - 1. High 🔼 These are important issues that should be resolved in the next release Component - C Library Core C library issues (usually in the src directory) Component - Fortran Fortran wrappers labels Jul 17, 2024

lrknox added the Merge - To 1.14 This needs to be merged to HDF5 1.14 label Jul 17, 2024

lrknox approved these changes Jul 17, 2024

View reviewed changes

qkoziol approved these changes Jul 17, 2024

View reviewed changes

brtnfld reviewed Jul 29, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/HDFGroup/hdf5 into fix_G…

92e5af3

…H-4447

brtnfld closed this Jul 29, 2024

brtnfld reopened this Jul 29, 2024

bmribler mentioned this pull request Aug 9, 2024

Inconsistent behavior of get_name functions in documentation #4704

Closed

bmribler and others added 4 commits August 11, 2024 14:29

Add test H5Aget_name against H5Rget_attr_name

9d57f6a

Committing clang-format changes

8be015a

Update documentation

da125e2

Fix typo

6edb92a

brtnfld reviewed Aug 12, 2024

View reviewed changes

brtnfld approved these changes Aug 12, 2024

View reviewed changes

lrknox approved these changes Aug 12, 2024

View reviewed changes

lrknox merged commit d875f74 into HDFGroup:develop Aug 12, 2024
55 checks passed

bmribler deleted the fix_GH-4447 branch August 13, 2024 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for H5R get name APIs #4657

Add tests for H5R get name APIs #4657

bmribler commented Jul 17, 2024 •

edited

Loading

jhendersonHDF commented Jul 29, 2024

brtnfld Jul 29, 2024 •

edited

Loading

bmribler Aug 12, 2024

bmribler commented Jul 29, 2024 •

edited

Loading

jhendersonHDF commented Jul 29, 2024

bmribler commented Jul 29, 2024

brtnfld commented Jul 29, 2024 •

edited

Loading

bmribler commented Jul 30, 2024

brtnfld commented Jul 30, 2024

bmribler commented Jul 30, 2024 •

edited

Loading

lrknox commented Aug 7, 2024

lrknox commented Aug 7, 2024

brtnfld commented Aug 7, 2024 •

edited by bmribler

Loading

bmribler commented Aug 7, 2024

bmribler commented Aug 9, 2024

brtnfld commented Aug 9, 2024

bmribler commented Aug 9, 2024

qkoziol commented Aug 10, 2024

brtnfld Aug 12, 2024

Add tests for H5R get name APIs #4657

Add tests for H5R get name APIs #4657

Conversation

bmribler commented Jul 17, 2024 • edited Loading

jhendersonHDF commented Jul 29, 2024

brtnfld Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

bmribler Aug 12, 2024

Choose a reason for hiding this comment

bmribler commented Jul 29, 2024 • edited Loading

jhendersonHDF commented Jul 29, 2024

bmribler commented Jul 29, 2024

brtnfld commented Jul 29, 2024 • edited Loading

bmribler commented Jul 30, 2024

brtnfld commented Jul 30, 2024

bmribler commented Jul 30, 2024 • edited Loading

lrknox commented Aug 7, 2024

lrknox commented Aug 7, 2024

brtnfld commented Aug 7, 2024 • edited by bmribler Loading

bmribler commented Aug 7, 2024

bmribler commented Aug 9, 2024

brtnfld commented Aug 9, 2024

bmribler commented Aug 9, 2024

qkoziol commented Aug 10, 2024

brtnfld Aug 12, 2024

Choose a reason for hiding this comment

bmribler commented Jul 17, 2024 •

edited

Loading

brtnfld Jul 29, 2024 •

edited

Loading

bmribler commented Jul 29, 2024 •

edited

Loading

brtnfld commented Jul 29, 2024 •

edited

Loading

bmribler commented Jul 30, 2024 •

edited

Loading

brtnfld commented Aug 7, 2024 •

edited by bmribler

Loading