From a7ff6427228918c5c09592c51cca4e68babed296 Mon Sep 17 00:00:00 2001 From: Jordan Henderson Date: Thu, 7 Mar 2024 16:59:35 -0600 Subject: [PATCH] Expand check for variable-length or reference types when clearing datatype conversion paths When clearing out datatype conversion paths involving variable-length or reference datatypes on file close, also check for these datatypes inside compound or array datatypes --- src/H5T.c | 90 ++++++++++++++++++++---- test/tmisc.c | 195 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 225 insertions(+), 60 deletions(-) diff --git a/src/H5T.c b/src/H5T.c index 09eeff8c2f1..9a3c321fd1e 100644 --- a/src/H5T.c +++ b/src/H5T.c @@ -350,12 +350,13 @@ static H5T_path_t *H5T__path_find_real(const H5T_t *src, const H5T_t *dst, const H5T_conv_func_t *conv); static bool H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5VL_object_t *owned_vol_obj, H5T_conv_t func); -static bool H5T__detect_vlen_ref(const H5T_t *dt); -static H5T_t *H5T__initiate_copy(const H5T_t *old_dt); -static H5T_t *H5T__copy_transient(H5T_t *old_dt); -static H5T_t *H5T__copy_all(H5T_t *old_dt); -static herr_t H5T__complete_copy(H5T_t *new_dt, const H5T_t *old_dt, H5T_shared_t *reopened_fo, - bool set_memory_type, H5T_copy_func_t copyfn); +static bool H5T_path_match_find_type_with_volobj(const H5T_t *datatype, const H5VL_object_t *owned_vol_obj); +static bool H5T__detect_vlen_ref(const H5T_t *dt); +static H5T_t *H5T__initiate_copy(const H5T_t *old_dt); +static H5T_t *H5T__copy_transient(H5T_t *old_dt); +static H5T_t *H5T__copy_all(H5T_t *old_dt); +static herr_t H5T__complete_copy(H5T_t *new_dt, const H5T_t *old_dt, H5T_shared_t *reopened_fo, + bool set_memory_type, H5T_copy_func_t copyfn); /*****************************/ /* Library Private Variables */ @@ -5183,10 +5184,10 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, { bool ret_value = true; - assert(path); - FUNC_ENTER_NOAPI_NOINIT_NOERR + assert(path); + if ( /* Check that the specified conversion function persistence matches */ ((H5T_PERS_SOFT == pers && path->is_hard) || (H5T_PERS_HARD == pers && !path->is_hard)) || @@ -5201,11 +5202,12 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, (src && H5T_cmp(src, path->src, false)) || (dst && H5T_cmp(dst, path->dst, false)) || /* - * Check that the specified VOL object matches the VOL object - * in the conversion path + * Check that the specified VOL object pointer matches the `owned_vol_obj` + * field for either the source datatype or destination datatype in the + * conversion path */ - (owned_vol_obj && (owned_vol_obj != path->src->shared->owned_vol_obj) && - (owned_vol_obj != path->dst->shared->owned_vol_obj)) || + (owned_vol_obj && (H5T_path_match_find_type_with_volobj(path->src, owned_vol_obj) == false) && + (H5T_path_match_find_type_with_volobj(path->dst, owned_vol_obj) == false)) || /* Check that the specified conversion function matches */ (func && func != path->conv.u.app_func)) @@ -5214,6 +5216,70 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, FUNC_LEAVE_NOAPI(ret_value) } /* H5T_path_match() */ +/*------------------------------------------------------------------------- + * Function: H5T_path_match_find_type_with_volobj + * + * Purpose: Helper function to determine whether a datatype is or + * contains a datatype that has a VOL object pointer matching + * the given VOL object pointer. + * + * Return: true/false (can't fail) + * + *------------------------------------------------------------------------- + */ +static bool +H5T_path_match_find_type_with_volobj(const H5T_t *datatype, const H5VL_object_t *owned_vol_obj) +{ + bool ret_value = false; + + FUNC_ENTER_NOAPI_NOINIT_NOERR + + assert(datatype); + assert(owned_vol_obj); + + ret_value = (datatype->shared->owned_vol_obj == owned_vol_obj); + if (!ret_value) { + switch (datatype->shared->type) { + case H5T_COMPOUND: + for (unsigned i = 0; i < datatype->shared->u.compnd.nmembs; i++) { + if (ret_value) + break; + ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->u.compnd.memb[i].type, + owned_vol_obj); + } + break; + + case H5T_VLEN: + /* Should be an error if no parent, but simplify logic for a true/false return value */ + if (datatype->shared->parent) + ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->parent, owned_vol_obj); + break; + + case H5T_ARRAY: + /* Should be an error if no parent, but simplify logic for a true/false return value */ + if (datatype->shared->parent) + ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->parent, owned_vol_obj); + break; + + case H5T_INTEGER: + case H5T_FLOAT: + case H5T_TIME: + case H5T_STRING: + case H5T_BITFIELD: + case H5T_OPAQUE: + case H5T_REFERENCE: /* Should have been determined by above check */ + case H5T_ENUM: + case H5T_NO_CLASS: /* Error value, but simplify logic for a true/false return value */ + case H5T_NCLASSES: /* Error value, but simplify logic for a true/false return value */ + default: + ret_value = false; + break; + } + } + + FUNC_LEAVE_NOAPI(ret_value) +} + /*------------------------------------------------------------------------- * Function: H5T_path_noop * diff --git a/test/tmisc.c b/test/tmisc.c index ddebc3d648f..4512e9ceccf 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -6273,17 +6273,32 @@ test_misc37(void) static void test_misc38(void) { - H5VL_object_t *file_vol_obj = NULL; - const char *buf[] = {"attr_value"}; - herr_t ret = SUCCEED; - hid_t file_id = H5I_INVALID_HID; - hid_t attr_id = H5I_INVALID_HID; - hid_t str_type = H5I_INVALID_HID; - hid_t space_id = H5I_INVALID_HID; - int init_npaths = 0; - int *irbuf = NULL; - char **rbuf = NULL; + H5VL_object_t *file_vol_obj = NULL; + const char *buf[] = {"attr_value"}; + const char *array_buf[] = {"attr_value1", "attr_value2"}; + hsize_t array_dims[1] = {2}; + herr_t ret = SUCCEED; + hid_t file_id = H5I_INVALID_HID; + hid_t attr_id1 = H5I_INVALID_HID; + hid_t attr_id2 = H5I_INVALID_HID; + hid_t attr_id3 = H5I_INVALID_HID; + hid_t attr_id4 = H5I_INVALID_HID; + hid_t str_type = H5I_INVALID_HID; + hid_t array_type = H5I_INVALID_HID; + hid_t compound_type = H5I_INVALID_HID; + hid_t vlen_type = H5I_INVALID_HID; + hid_t space_id = H5I_INVALID_HID; + int init_npaths = 0; + char **rbuf = NULL; + char **arr_rbuf = NULL; bool vol_is_native; + typedef struct struct_type { + const char *buf; + } struct_type; + struct_type cbuf = {.buf = "attr_value"}; + struct_type *compound_rbuf = NULL; + hvl_t vlen_buf = {.len = 2, .p = array_buf}; + hvl_t *vlen_rbuf = NULL; /* Output message about test being performed */ MESSAGE(5, ("Fix for type conversion path table issue")); @@ -6315,12 +6330,27 @@ test_misc38(void) */ VERIFY(file_vol_obj->rc, 1, "checking reference count"); + /* Create a variable-length string type */ str_type = H5Tcopy(H5T_C_S1); CHECK(str_type, H5I_INVALID_HID, "H5Tcopy"); ret = H5Tset_size(str_type, H5T_VARIABLE); CHECK(ret, FAIL, "H5Tset_size"); + /* Create an array type of the string type */ + array_type = H5Tarray_create2(str_type, 1, array_dims); + CHECK(array_type, H5I_INVALID_HID, "H5Tarray_create2"); + + /* Create a compound type of the string type */ + compound_type = H5Tcreate(H5T_COMPOUND, sizeof(compound_type)); + CHECK(compound_type, H5I_INVALID_HID, "H5Tcreate(H5T_COMPOUND, ...)"); + + CHECK(H5Tinsert(compound_type, "varstr", HOFFSET(struct_type, buf), str_type), FAIL, "H5Tinsert"); + + /* Create a variable-length type of the string type */ + vlen_type = H5Tvlen_create(str_type); + CHECK(vlen_type, H5I_INVALID_HID, "H5Tvlen_create"); + space_id = H5Screate(H5S_SCALAR); CHECK(space_id, H5I_INVALID_HID, "H5Screate"); @@ -6331,8 +6361,19 @@ test_misc38(void) VERIFY(H5T__get_path_table_npaths(), init_npaths, "checking number of type conversion path table entries"); - attr_id = H5Acreate2(file_id, "attribute", str_type, space_id, H5P_DEFAULT, H5P_DEFAULT); - CHECK(attr_id, H5I_INVALID_HID, "H5Acreate2"); + /* Increments file's VOL object reference count by 1 */ + attr_id1 = H5Acreate2(file_id, "varstr_attribute", str_type, space_id, H5P_DEFAULT, H5P_DEFAULT); + CHECK(attr_id1, H5I_INVALID_HID, "H5Acreate2"); + /* Increments file's VOL object reference count by 1 */ + attr_id2 = H5Acreate2(file_id, "array_varstr_attribute", array_type, space_id, H5P_DEFAULT, H5P_DEFAULT); + CHECK(attr_id2, H5I_INVALID_HID, "H5Acreate2"); + /* Increments file's VOL object reference count by 1 */ + attr_id3 = + H5Acreate2(file_id, "compound_varstr_attribute", compound_type, space_id, H5P_DEFAULT, H5P_DEFAULT); + CHECK(attr_id3, H5I_INVALID_HID, "H5Acreate2"); + /* Increments file's VOL object reference count by 2 */ + attr_id4 = H5Acreate2(file_id, "vlen_varstr_attribute", vlen_type, space_id, H5P_DEFAULT, H5P_DEFAULT); + CHECK(attr_id4, H5I_INVALID_HID, "H5Acreate2"); /* * Check the number of type conversion path table entries currently @@ -6343,44 +6384,64 @@ test_misc38(void) /* * Check reference count of file's VOL object field. At this point, - * the object should have a reference count of 2. Creating the - * attribute on the dataset will have caused a H5T_set_loc call that - * associates the attribute's datatype with the file's VOL object - * and will have incremented the reference count by 1. + * the object should have a reference count of 6. Creating the + * attributes in the file will have caused H5T_set_loc calls that + * associate each attribute's datatype with the file's VOL object + * and will have incremented the reference count by 5. */ - VERIFY(file_vol_obj->rc, 2, "checking reference count"); + VERIFY(file_vol_obj->rc, 6, "checking reference count"); - ret = H5Awrite(attr_id, str_type, buf); + /* Increments file's VOL object reference count by 1 */ + ret = H5Awrite(attr_id1, str_type, buf); + CHECK(ret, FAIL, "H5Awrite"); + /* Increments file's VOL object reference count by 1 */ + ret = H5Awrite(attr_id2, array_type, array_buf); + CHECK(ret, FAIL, "H5Awrite"); + /* Increments file's VOL object reference count by 2 */ + ret = H5Awrite(attr_id3, compound_type, &cbuf); + CHECK(ret, FAIL, "H5Awrite"); + /* Increments file's VOL object reference count by 2 */ + ret = H5Awrite(attr_id4, vlen_type, &vlen_buf); CHECK(ret, FAIL, "H5Awrite"); /* * Check the number of type conversion path table entries currently - * stored in the cache. The H5Awrite call should have added a new - * type conversion path. Note that if another test in this file uses - * the same conversion path, this check may fail and need to be - * refactored. + * stored in the cache. The H5Awrite calls should have added new + * type conversion paths. Note that if another test in this file + * uses the same conversion path, this check may fail and need to + * be refactored. */ - VERIFY(H5T__get_path_table_npaths(), init_npaths + 1, + VERIFY(H5T__get_path_table_npaths(), init_npaths + 4, "checking number of type conversion path table entries"); /* * Check reference count of file's VOL object field. At this point, - * the object should have a reference count of 3. Writing to the - * variable-length typed attribute will have caused an H5T_convert - * call that ends up incrementing the reference count of the - * associated file's VOL object. + * the object should have a reference count of 12. Writing to the + * attributes will have caused H5T_path_find calls that end up + * incrementing the reference count of the associated file's VOL + * object. */ - VERIFY(file_vol_obj->rc, 3, "checking reference count"); + VERIFY(file_vol_obj->rc, 12, "checking reference count"); - ret = H5Aclose(attr_id); + ret = H5Aclose(attr_id1); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id2); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id3); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id4); CHECK(ret, FAIL, "H5Aclose"); ret = H5Fclose(file_id); CHECK(ret, FAIL, "H5Fclose"); - irbuf = malloc(100 * 100 * sizeof(int)); - CHECK_PTR(irbuf, "int read buf allocation"); rbuf = malloc(sizeof(char *)); CHECK_PTR(rbuf, "varstr read buf allocation"); + arr_rbuf = malloc(array_dims[0] * sizeof(char *)); + CHECK_PTR(arr_rbuf, "array varstr read buf allocation"); + compound_rbuf = malloc(sizeof(struct_type)); + CHECK_PTR(compound_rbuf, "compound varstr read buf allocation"); + vlen_rbuf = malloc(sizeof(hvl_t)); + CHECK_PTR(vlen_rbuf, "vlen varstr read buf allocation"); for (size_t i = 0; i < 10; i++) { file_id = H5Fopen(MISC38_FILE, H5F_ACC_RDONLY, H5P_DEFAULT); @@ -6396,51 +6457,89 @@ test_misc38(void) */ VERIFY(file_vol_obj->rc, 1, "checking reference count"); - attr_id = H5Aopen(file_id, "attribute", H5P_DEFAULT); - CHECK(attr_id, H5I_INVALID_HID, "H5Aopen"); + /* Increments file's VOL object reference count by 1 */ + attr_id1 = H5Aopen(file_id, "varstr_attribute", H5P_DEFAULT); + CHECK(attr_id1, H5I_INVALID_HID, "H5Aopen"); + /* Increments file's VOL object reference count by 1 */ + attr_id2 = H5Aopen(file_id, "array_varstr_attribute", H5P_DEFAULT); + CHECK(attr_id2, H5I_INVALID_HID, "H5Aopen"); + /* Increments file's VOL object reference count by 1 */ + attr_id3 = H5Aopen(file_id, "compound_varstr_attribute", H5P_DEFAULT); + CHECK(attr_id3, H5I_INVALID_HID, "H5Aopen"); + /* Increments file's VOL object reference count by 2 */ + attr_id4 = H5Aopen(file_id, "vlen_varstr_attribute", H5P_DEFAULT); + CHECK(attr_id4, H5I_INVALID_HID, "H5Aopen"); /* * Check reference count of file's VOL object field. At this point, - * the object should have a reference count of 2 since opening - * the attribute will also have associated its type with the file's - * VOL object. + * the object should have a reference count of 6 since opening + * the attributes will also have associated their datatypes with + * the file's VOL object. */ - VERIFY(file_vol_obj->rc, 2, "checking reference count"); + VERIFY(file_vol_obj->rc, 6, "checking reference count"); - ret = H5Aread(attr_id, str_type, rbuf); + /* Increments file's VOL object reference count by 1 */ + ret = H5Aread(attr_id1, str_type, rbuf); + CHECK(ret, FAIL, "H5Aread"); + /* Increments file's VOL object reference count by 1 */ + ret = H5Aread(attr_id2, array_type, arr_rbuf); + CHECK(ret, FAIL, "H5Aread"); + /* Increments file's VOL object reference count by 2 */ + ret = H5Aread(attr_id3, compound_type, compound_rbuf); + CHECK(ret, FAIL, "H5Aread"); + /* Increments file's VOL object reference count by 2 */ + ret = H5Aread(attr_id4, vlen_type, vlen_rbuf); CHECK(ret, FAIL, "H5Aread"); /* * Check the number of type conversion path table entries currently * stored in the cache. Each H5Aread call shouldn't cause this number - * to go up, as the library should have removed the cached conversion - * paths on file close. + * to keep going up, as the library should remove the cached conversion + * paths on file close during each iteration. The value should stay at + * a constant "initial_num_paths + number of H5Aread calls above". */ - VERIFY(H5T__get_path_table_npaths(), init_npaths + 1, + VERIFY(H5T__get_path_table_npaths(), init_npaths + 4, "checking number of type conversion path table entries"); /* * Check reference count of file's VOL object field. At this point, - * the object should have a reference count of 3. Writing to the - * variable-length typed attribute will have caused an H5T_convert - * call that ends up incrementing the reference count of the - * associated file's VOL object. + * the object should have a reference count of 12. Reading from the + * attributes will have caused H5T_path_find calls that end up + * incrementing the reference count of the associated file's VOL + * object. */ - VERIFY(file_vol_obj->rc, 3, "checking reference count"); + VERIFY(file_vol_obj->rc, 12, "checking reference count"); ret = H5Treclaim(str_type, space_id, H5P_DEFAULT, rbuf); + ret = H5Treclaim(array_type, space_id, H5P_DEFAULT, arr_rbuf); + ret = H5Treclaim(compound_type, space_id, H5P_DEFAULT, compound_rbuf); + ret = H5Treclaim(vlen_type, space_id, H5P_DEFAULT, vlen_rbuf); - ret = H5Aclose(attr_id); + ret = H5Aclose(attr_id1); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id2); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id3); + CHECK(ret, FAIL, "H5Aclose"); + ret = H5Aclose(attr_id4); CHECK(ret, FAIL, "H5Aclose"); ret = H5Fclose(file_id); CHECK(ret, FAIL, "H5Fclose"); } - free(irbuf); free(rbuf); + free(arr_rbuf); + free(compound_rbuf); + free(vlen_rbuf); ret = H5Tclose(str_type); CHECK(ret, FAIL, "H5Tclose"); + ret = H5Tclose(array_type); + CHECK(ret, FAIL, "H5Tclose"); + ret = H5Tclose(compound_type); + CHECK(ret, FAIL, "H5Tclose"); + ret = H5Tclose(vlen_type); + CHECK(ret, FAIL, "H5Tclose"); ret = H5Sclose(space_id); CHECK(ret, FAIL, "H5Sclose"); }