Remove some internal use of API calls and H5E_BEGIN_TRY/H5E_END_TRY #4624
Conversation
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
qkoziol
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
jhendersonHDF,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
| /* Store current error stack size */ | ||
| if ((num_errors = H5Eget_num(H5E_DEFAULT)) < 0) | ||
| HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, H5_ITER_ERROR, "can't get current error stack size"); | ||
| if (H5VL_file_specific(NULL, &vol_cb_args, H5P_DATASET_XFER_DEFAULT, H5_REQUEST_NULL) < 0) |
There was a problem hiding this comment.
This seems like another case where we need this to be a 'try'. The previous code was treating it like a 'try' by popping any errors that occurred from the error stack if the is_accessible call failed, but trying to keep the error messages (if any) around in the case where the file isn't accessible. Now if the is_accessible call fails, it just fails the iteration, which is not quite the behavior wanted here.
There was a problem hiding this comment.
It is effectively a 'try' already - it's returning the flag in a parameter. So, any failure means that the flag couldn't be determined, which is a real error. I think that a file not being accessible isn't an error, it's just not accessible.
There was a problem hiding this comment.
The original intention (for better or worse) was to ignore errors that occur during the H5VL_file_specific call, treat them as the file not being accessible with the current VOL connector and then move on to the next one so that each available connector is tried. IIRC, the native VOL used to return FAIL when a file wasn't accessible, but I believe that has been fixed so it will return false now. I'm not sure whether or not the expected behavior of H5VL_file_specific in terms of FAIL vs false is really documented though.
There was a problem hiding this comment.
I believe that it would be more consistent to have this routine act like the rest of the library's 'try' routines, where it is an actual error to not be able to answer the question "is this file accessible?". That was the original intention of the 'is_accessible' callback. As you said, the native VOL connector was already discriminating between actual errors and 'false', so it's behaving correctly.
There was a problem hiding this comment.
I'm not particularly stuck on the current implementation since this is functionality that is mostly unused outside of niche cases, but it would definitely be good to make sure that the distinction between FAIL and false for the is_accessible callback is documented appropriately.
There was a problem hiding this comment.
Yes, definitely, I'll add some blurbs for it in the morning.
There was a problem hiding this comment.
@jhendersonHDF - How's this look?
src/H5Fint.c
Outdated
|
|
||
| FUNC_ENTER_PACKAGE | ||
|
|
||
| /* Reset output parameter */ | ||
| *is_hdf5 = false; |
There was a problem hiding this comment.
This would potentially modify the argument even in the case of failure.
There was a problem hiding this comment.
Yes, I considered that, but the previous behavior was aggregating FAIL with 'false' and it seemed best to continue that.
There was a problem hiding this comment.
It looks like H5VL__native_file_specific() previously would not touch the user's buffer if H5F__is_hdf5() failed, whereas with this change it would, since the user buffer is passed through directly to H5F__is_hdf5(). Also, since it wouldn't be hard to do I think we should just follow the normal pattern where output parameters aren't changed in case of failure. We would of course need another variable here to handle the error check in the close section.
There was a problem hiding this comment.
OK, I'll update the code.
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
derobins
added
Merge - To 1.14
Priority - 1. High 🔼
Signed-off-by: Quincey Koziol <quincey@koziol.cc>
* Test fixes for log-based vol (#4618) * fixes to address failures in the log-based VOL * moved file cleanup to tests proper * skipped index API test if not supported * Add 'try' parameter to H5Z_find, and remove calls to H5E_clear_stack() (#4609) * Bump the github-actions group with 4 updates (#4620) Bumps the github-actions group with 4 updates: [actions/checkout](https://github.com/actions/checkout), [aws-actions/configure-aws-credentials](https://github.com/aws-actions/configure-aws-credentials), [softprops/action-gh-release](https://github.com/softprops/action-gh-release) and [github/codeql-action](https://github.com/github/codeql-action). Updates `actions/checkout` from 4.1.1 to 4.1.7 - [Release notes](https://github.com/actions/checkout/releases) - [Commits](actions/checkout@v4.1.1...v4.1.7) Updates `aws-actions/configure-aws-credentials` from 1 to 4 - [Release notes](https://github.com/aws-actions/configure-aws-credentials/releases) - [Changelog](https://github.com/aws-actions/configure-aws-credentials/blob/main/CHANGELOG.md) - [Commits](aws-actions/configure-aws-credentials@v1...v4) Updates `softprops/action-gh-release` from 2.0.5 to 2.0.6 - [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@69320db...a74c6b7) Updates `github/codeql-action` from 3.25.7 to 3.25.11 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@f079b84...b611370) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions - dependency-name: aws-actions/configure-aws-credentials dependency-type: direct:production update-type: version-update:semver-major 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: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix a stack size warning in ros3 VFD code (#4625) Just makes a stack array dynamic. Valgrind shows no memory leaks. * Reworked cleaning up API test files (#4626) * Reworked cleaning up test files, only removing test files if present to account for skipped tests * changed to using H5Fis_accessible * update to full use of remove_test_file * corrected offset type in C wrapper (#4622) * Remove some internal use of API calls and H5E_BEGIN_TRY/H5E_END_TRY (#4624) * Remove auto NSIS check because of issues with CI (#4646) * Add python testing for examples (#4628) * Clean up Fortran __float128 configure-time checks (#4649) * Always use DECIMAL_DIG instead of LDBL_DIG. This was controlled by an ifdef that is always true in C99 or greater It's confusing to use float.h C constants as variable names in configure.ac and the PAC_FC_LDBL_DIG macro. * Directly compare MY_FLT128_DIG and MY_LDBL_DIG * Make uniform across CMake and Autotools * Don't export quadmath.h variables to H5pubconf.h * Feature/awesome (#4604) * Added Doxygen Awesome and fixed a few quirks. * Fixed unterminated strings. * Added Doxygen Awesome by copy. * Add tools usage text as doxygen for Tools UG (#4602) * Add h5* compiler wrapper testing for CMake #4605 (#4613) * Add show option * remove non-static libs and correct names of static libs * Fixup the pkg-config libs and comp builds * Fix commands and add fortran pkg-config test scripts * Add help usage option * Add temporary fix for ARM64 Mac _Float16 build failure (#4639) * Correct H5VL_t ref count on H5O_refresh_metadata failure (#4636) * Fix bad H5VL_t rc on H5O_refresh_metadata fail * Decrement nrefs before raising error * Update doxygen Learn Basics / example refs. Add Reference sections (#4640) * Fixed messed up table captions. (#4653) * Fixed messed up table captions. Browsers don't seem to respect relative values for width. Hardcoding 800px for now. * Fixed FetchContent usage for new CMake reqs. (#4650) CMake version 3.30 changed the behavior of the FetchContent module to deprecate the use of FetchContent_Populate() in favor of FetchContent_MakeAvailable(). Therefore, the copying of HDF specialized CMakeLists.txt files to the dependent project's source was implemented in the FetchContent_Declare() call. * Fixed usage issue with FindZLIB.cmake module (#4655) * Add a comment on the FindZLIB.cmake module usage * Allow choice of static/shared compression libs for Find Module * Added new option to INSTALL_CMake file and changed option text * Eliminate more H5E_BEGIN/END_TRY macros and H5E_clear_stack() calls (#4648) * Correct name of zlib_ng option (#4658) * Fix the examples for testing java with binaries (#4660) * Update filename in RELEASE_PROCESS.md to current name INSTALL_autotools.txt. * Remove reference to V116 in tools/src/h5repack/h5repack.h.
No description provided.