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.

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

Updated H5VOL for HDF5 v1.13 #2945

Closed
wants to merge 15 commits into from
Closed

Updated H5VOL for HDF5 v1.13 #2945

wants to merge 15 commits into from

Conversation

brtnfld
Copy link
Contributor

@brtnfld brtnfld commented Nov 3, 2021

Updated H5VOL for HDF5 v1.13 release, dropping support for v1.12 (recommended).

@@ -53,7 +51,8 @@ static const H5VL_class_t H5VL_adios2_def = {
H5VL_ADIOS2_VERSION,
(H5VL_class_value_t)H5VL_ADIOS2_VALUE,
H5VL_ADIOS2_NAME, /* name */
0,
0, /* Version # of connector */
0, /* Capability flags for connector */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also use H5VL_CAP_FLAG_NONE here.

char *buf = va_arg(arguments, char *);
ssize_t *ret_val = va_arg(arguments, ssize_t *);
char *buf = args->args.get_name.buf;
ssize_t *ret_val = (ssize_t*)args->args.get_name.attr_name_len;
Copy link
Contributor

Choose a reason for hiding this comment

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

This changed to a size_t * with changes in the VOL.

{
REQUIRE_NOT_NULL_ERR(obj, -1);
H5VL_ObjDef_t *vol = (H5VL_ObjDef_t *)obj;
const char *attr_name = va_arg(arguments, const char *);
const char *attr_name = (const char *)args->args.del.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

It won't necessarily be safe to retrieve the attribute's name here until you know that the delete operation is the one occurring since the arguments are in a union.

For delete, the attribute name is in args->args.del.name, for exists, it's in args->args.exists.name and for rename it's in args->args.rename.old_name for example.

char *name = va_arg(arguments, char *);
ssize_t *ret = va_arg(arguments, ssize_t *);
char *name = args->args.get_name.name;
ssize_t *ret = (ssize_t*)args->args.get_name.name_len;
Copy link
Contributor

Choose a reason for hiding this comment

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

Changed to a size_t * with changes in the VOL.

@@ -33,10 +33,8 @@ extern herr_t H5VL_adios2_beginstep(const char *engine_name,

extern herr_t H5VL_adios2_endstep(const char *engine_nane);

static herr_t H5VL_adios2_introspect_opt_query(void *obj, H5VL_subclass_t cls,
int opt_type, hbool_t *supported)
Copy link
Contributor

Choose a reason for hiding this comment

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

This old signature is actually the correct one, with the hbool_t * changed to a uint64_t * instead.

}

const H5VL_loc_params_t *loc_params =
va_arg(arguments, const H5VL_loc_params_t *);
const H5VL_loc_params_t *loc_params = &args->args.get_info.loc_params;
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 probably move this inside the get_info block so that you don't try to access the get_info portion when that's not the operation being performed.

@brtnfld brtnfld marked this pull request as ready for review December 6, 2021 19:42
@brtnfld
Copy link
Contributor Author

brtnfld commented Dec 6, 2021

HDF5 version 1.13.0 is being released today.

@brtnfld
Copy link
Contributor Author

brtnfld commented Dec 6, 2021

Changed CI to use 1.13.0 release instead of 1.12.0

@brtnfld
Copy link
Contributor Author

brtnfld commented Dec 9, 2021

It looks like the Docker images of the failed tests need to be updated to HDF5 1.13. I'm not sure how to do that.

@pnorbert
Copy link
Contributor

It looks like the Docker images of the failed tests need to be updated to HDF5 1.13. I'm not sure how to do that.
@chuckatkins Can you please help with the docker images. Thanks.

@brtnfld
Copy link
Contributor Author

brtnfld commented Dec 14, 2021

@chuckatkins I added a check for HDF5 > 1.12

@chuckatkins
Copy link
Contributor

Replaced by #3077

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants