From 05cca142f24b3cf764bc760bdd3baf2e63e9870f Mon Sep 17 00:00:00 2001 From: Dana Robinson <43805+derobins@users.noreply.github.com> Date: Mon, 18 Mar 2024 06:57:52 -0700 Subject: [PATCH] Allow H5Soffset_simple to accept NULL offsets (#4152) The reference manual states that the offset parameter of H5Soffset_simple() can be set to NULL to reset the offset of a simple dataspace to 0. This has never been true, and passing NULL was regarded as an error. The library will now accept NULL for the offset parameter and will correctly set the offset to zero. Fixes HDFFV-9299 --- release_docs/RELEASE.txt | 11 +++++++++++ src/H5Spkg.h | 1 + src/H5Spublic.h | 6 +++++- src/H5Sselect.c | 23 +++++++++++----------- src/H5Stest.c | 42 +++++++++++++++++++++++++++++++++++----- test/tselect.c | 26 +++++++++++++++++++++++++ 6 files changed, 91 insertions(+), 18 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 02541b34dac..c6f656a61a7 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -281,6 +281,17 @@ Bug Fixes since HDF5-1.14.3 release Library ------- + - Corrected H5Soffset_simple() when offset is NULL + + The reference manual states that the offset parameter of H5Soffset_simple() + can be set to NULL to reset the offset of a simple dataspace to 0. This + has never been true, and passing NULL was regarded as an error. + + The library will now accept NULL for the offset parameter and will + correctly set the offset to zero. + + Fixes HDFFV-9299 + - Fixed an issue where the Subfiling VFD's context object cache could grow too large diff --git a/src/H5Spkg.h b/src/H5Spkg.h index 3ae63a50964..fddd5e3c5fa 100644 --- a/src/H5Spkg.h +++ b/src/H5Spkg.h @@ -413,6 +413,7 @@ H5_DLL herr_t H5S__get_rebuild_status_test(hid_t space_id, H5S_diminfo_valid_t * H5S_diminfo_valid_t *status2); H5_DLL herr_t H5S__get_diminfo_status_test(hid_t space_id, H5S_diminfo_valid_t *status); H5_DLL htri_t H5S__internal_consistency_test(hid_t space_id); +H5_DLL herr_t H5S__verify_offsets(hid_t space_id, const hssize_t *offset); #endif /* H5S_TESTING */ #endif /*H5Spkg_H*/ diff --git a/src/H5Spublic.h b/src/H5Spublic.h index 5422d96bcbf..bf9e9118f64 100644 --- a/src/H5Spublic.h +++ b/src/H5Spublic.h @@ -832,12 +832,16 @@ H5_DLL herr_t H5Smodify_select(hid_t space1_id, H5S_seloper_t op, hid_t space2_i * \p space_id. The offset array must be the same number of * elements as the number of dimensions for the dataspace. If the * \p offset array is set to NULL, the offset for the dataspace is - * reset to 0. + * reset to 0 in all dimensions. * * This function allows the same shaped selection to be moved to * different locations within a dataspace without requiring it to * be redefined. * + * \note Until 1.14.4, setting the offset parameter to NULL was considered + * an error, despite the reference manual stating that it had the + * behavior described above. + * * \version 1.4.0 Fortran subroutine was introduced. * \since 1.0.0 * diff --git a/src/H5Sselect.c b/src/H5Sselect.c index 477e0f840c1..7a032b26e85 100644 --- a/src/H5Sselect.c +++ b/src/H5Sselect.c @@ -81,11 +81,6 @@ H5FL_SEQ_EXTERN(hsize_t); Non-negative on success/Negative on failure DESCRIPTION Sets the selection offset for the dataspace - GLOBAL VARIABLES - COMMENTS, BUGS, ASSUMPTIONS - Only works for simple dataspaces currently - EXAMPLES - REVISION LOG --------------------------------------------------------------------------*/ herr_t H5S_select_offset(H5S_t *space, const hssize_t *offset) @@ -95,10 +90,14 @@ H5S_select_offset(H5S_t *space, const hssize_t *offset) /* Check args */ assert(space); assert(0 < space->extent.rank && space->extent.rank <= H5S_MAX_RANK); - assert(offset); - /* Copy the offset over */ - H5MM_memcpy(space->select.offset, offset, sizeof(hssize_t) * space->extent.rank); + /* Copy the offset over. As a special case, when offset is NULL, we + * reset all dimensions to zero. + */ + if (offset) + H5MM_memcpy(space->select.offset, offset, sizeof(hssize_t) * space->extent.rank); + else + memset(space->select.offset, 0, sizeof(hssize_t) * space->extent.rank); /* Indicate that the offset was changed */ space->select.offset_changed = true; @@ -133,12 +132,12 @@ H5Soffset_simple(hid_t space_id, const hssize_t *offset) /* Check args */ if (NULL == (space = (H5S_t *)H5I_object_verify(space_id, H5I_DATASPACE))) - HGOTO_ERROR(H5E_ID, H5E_BADID, FAIL, "not a dataspace"); + HGOTO_ERROR(H5E_DATASPACE, H5E_BADID, FAIL, "not a dataspace"); if (space->extent.rank == 0 || (H5S_GET_EXTENT_TYPE(space) == H5S_SCALAR || H5S_GET_EXTENT_TYPE(space) == H5S_NULL)) - HGOTO_ERROR(H5E_ID, H5E_UNSUPPORTED, FAIL, "can't set offset on scalar or null dataspace"); - if (offset == NULL) - HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, FAIL, "no offset specified"); + HGOTO_ERROR(H5E_DATASPACE, H5E_UNSUPPORTED, FAIL, "can't set offset on scalar or null dataspace"); + + /* offset can be NULL (resets all dims to zero) */ /* Set the selection offset */ if (H5S_select_offset(space, offset) < 0) diff --git a/src/H5Stest.c b/src/H5Stest.c index 4978de0558e..422f7c1d20f 100644 --- a/src/H5Stest.c +++ b/src/H5Stest.c @@ -341,11 +341,6 @@ H5S__check_internal_consistency(const H5S_t *space) DESCRIPTION Check the states of internal data structures of the hyperslab, and see whether they are consistent or not - GLOBAL VARIABLES - COMMENTS, BUGS, ASSUMPTIONS - DO NOT USE THIS FUNCTION FOR ANYTHING EXCEPT TESTING - EXAMPLES - REVISION LOG --------------------------------------------------------------------------*/ htri_t H5S__internal_consistency_test(hid_t space_id) @@ -367,3 +362,40 @@ H5S__internal_consistency_test(hid_t space_id) done: FUNC_LEAVE_NOAPI(ret_value) } /* H5S__internal_consistency_test() */ + +/*-------------------------------------------------------------------------- + NAME + H5S__verify_offsets + PURPOSE + Verify that internal offsets match an array of offsets + USAGE + herr_t H5S__verify_offsets(hid_t space_id) + hid_t space_id; IN: dataspace id + const hssize_t *offset; IN: Offset to position the selection at + RETURNS + Non-negative true/false on success, negative on failure + DESCRIPTION + This function is necessary because there is no public API call + that lets you get the offsets +--------------------------------------------------------------------------*/ +herr_t +H5S__verify_offsets(hid_t space_id, const hssize_t *offset) +{ + H5S_t *space; /* Dataspace to modify */ + herr_t ret_value = SUCCEED; /* Return value */ + + FUNC_ENTER_PACKAGE + + if (NULL == (space = (H5S_t *)H5I_object_verify(space_id, H5I_DATASPACE))) + HGOTO_ERROR(H5E_DATASPACE, H5E_BADID, FAIL, "not a dataspace"); + if (space->extent.rank == 0 || + (H5S_GET_EXTENT_TYPE(space) == H5S_SCALAR || H5S_GET_EXTENT_TYPE(space) == H5S_NULL)) + HGOTO_ERROR(H5E_DATASPACE, H5E_UNSUPPORTED, FAIL, "can't set offset on scalar or null dataspace"); + + /* Check that the internal and passed-in offset data are the same */ + if (0 != memcmp(space->select.offset, offset, sizeof(hssize_t) * space->extent.rank)) + HGOTO_ERROR(H5E_DATASPACE, H5E_BADVALUE, FAIL, "internal offsets don't match parameters"); + +done: + FUNC_LEAVE_NOAPI(ret_value) +} /* end H5S__verify_offsets() */ diff --git a/test/tselect.c b/test/tselect.c index 55430f24ad5..20b85916739 100644 --- a/test/tselect.c +++ b/test/tselect.c @@ -4118,6 +4118,8 @@ test_select_hyper_offset(void) CHECK(ret, FAIL, "H5Soffset_simple"); valid = H5Sselect_valid(sid1); VERIFY(valid, true, "H5Sselect_valid"); + ret = H5S__verify_offsets(sid1, offset); + CHECK(ret, FAIL, "H5S__verify_offsets"); /* Check an invalid offset */ offset[0] = 10; @@ -4127,6 +4129,8 @@ test_select_hyper_offset(void) CHECK(ret, FAIL, "H5Soffset_simple"); valid = H5Sselect_valid(sid1); VERIFY(valid, false, "H5Sselect_valid"); + ret = H5S__verify_offsets(sid1, offset); + CHECK(ret, FAIL, "H5S__verify_offsets"); /* Reset offset */ offset[0] = 0; @@ -4136,6 +4140,28 @@ test_select_hyper_offset(void) CHECK(ret, FAIL, "H5Soffset_simple"); valid = H5Sselect_valid(sid1); VERIFY(valid, true, "H5Sselect_valid"); + ret = H5S__verify_offsets(sid1, offset); + CHECK(ret, FAIL, "H5S__verify_offsets"); + + /* Check behavior of NULL offset parameter */ + + /* Set a valid offset */ + offset[0] = -1; + offset[1] = 0; + offset[2] = 0; + ret = H5Soffset_simple(sid1, offset); + CHECK(ret, FAIL, "H5Soffset_simple"); + valid = H5Sselect_valid(sid1); + VERIFY(valid, true, "H5Sselect_valid"); + /* Reset using NULL */ + ret = H5Soffset_simple(sid1, NULL); + CHECK(ret, FAIL, "H5Soffset_simple"); + valid = H5Sselect_valid(sid1); + VERIFY(valid, true, "H5Sselect_valid"); + /* Validate offset */ + offset[0] = 0; + ret = H5S__verify_offsets(sid1, offset); + CHECK(ret, FAIL, "H5S__verify_offsets"); /* Select 15x26 hyperslab for memory dataset */ start[0] = 15;