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.

Sign up for GitHub

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

Jump to bottom

Expand check for variable-length or reference types when clearing datatype conversion paths #4085

Merged
merged 1 commit into from
Mar 9, 2024
Merged

Expand check for variable-length or reference types when clearing datatype conversion paths #4085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 78 additions & 12 deletions src/H5T.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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)) ||
Expand All @@ -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))
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think that the ENUM & VLEN parent (base) types should also be checked, why would they be covered by the earlier check?

Copy link
Collaborator Author

@jhendersonHDF jhendersonHDF Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For enum, the base type can only be an integer type, so we shouldn't need to check that since only vlen and reference types will have a VOL object field set. But good point on the vlen of vlen; I wasn't thinking recursively enough!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the base type of the enum is a VOL object? (I think the answer is yes, but haven't looked in detail)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the base type of the enum is a VOL object? (I think the answer is yes, but haven't looked in detail)

I suppose with a committed datatype you might have an enum where the base type is wrapped in a VOL object, but this function is only about checking the owned_vol_obj field in a datatype's shared structure, which only ever gets set for variable-length and reference datatypes. Adding a check against the base type of an enum shouldn't really hurt anything, but the enum type and its base type should never have the owned_vol_obj field set.

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
*
Expand Down
Loading
Loading