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

Remove some internal use of API calls and H5E_BEGIN_TRY/H5E_END_TRY #4624

Merged
merged 9 commits into from
Jul 12, 2024
Merged

Remove some internal use of API calls and H5E_BEGIN_TRY/H5E_END_TRY #4624

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b8a6800
Remove some internal use of API calls and H5E_BEGIN_TRY/H5E_END_TRY
qkoziol Jul 2, 2024
bfb055d
Clean up warnings
qkoziol Jul 2, 2024
057e52d
Committing clang-format changes
github-actions[bot] Jul 3, 2024
10b1efc
Add additional documentation for the 'is_accessible' operation
qkoziol Jul 3, 2024
5b3592e
Merge remote-tracking branch 'origin/develop' into pause_errors_4
qkoziol Jul 5, 2024
733ef55
Avoid changing the output parameter
qkoziol Jul 9, 2024
bd19731
Committing clang-format changes
github-actions[bot] Jul 9, 2024
54bc98a
Fix typo
qkoziol Jul 9, 2024
9c0881c
Committing clang-format changes
github-actions[bot] Jul 9, 2024
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
17 changes: 10 additions & 7 deletions src/H5Fint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,19 +1029,22 @@ H5F_prefix_open_file(bool try, H5F_t **_file, H5F_t *primary_file, H5F_prefix_op
*
* Purpose: Check the file signature to detect an HDF5 file.
*
* Return: true/false/FAIL
* Return: SUCCEED/FAIL
*-------------------------------------------------------------------------
*/
htri_t
H5F__is_hdf5(const char *name, hid_t fapl_id)
herr_t
H5F__is_hdf5(const char *name, hid_t fapl_id, bool *is_hdf5)
{
H5FD_t *lf = NULL; /* Low-level file struct */
H5F_shared_t *shared = NULL; /* Shared part of file */
haddr_t sig_addr = HADDR_UNDEF; /* Address of hdf5 file signature */
htri_t ret_value = FAIL; /* Return value */
herr_t ret_value = SUCCEED; /* Return value */

FUNC_ENTER_PACKAGE

/* Reset output parameter */
*is_hdf5 = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This would potentially modify the argument even in the case of failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I considered that, but the previous behavior was aggregating FAIL with 'false' and it seemed best to continue that.

Copy link
Member

@fortnern fortnern Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll update the code.


/* Open the file */
/* NOTE: This now uses the fapl_id that was passed in, so H5Fis_accessible()
* should work with arbitrary VFDs, unlike H5Fis_hdf5().
Expand All @@ -1056,18 +1059,18 @@ H5F__is_hdf5(const char *name, hid_t fapl_id)
* to read through it will fail so we have to try this first.
*/
if (NULL != (shared = H5F__sfile_search(lf)))
ret_value = true;
*is_hdf5 = true;
else {
/* The file is an HDF5 file if the HDF5 file signature can be found */
if (H5FD_locate_signature(lf, &sig_addr) < 0)
HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "error while trying to locate file signature");
ret_value = (HADDR_UNDEF != sig_addr);
*is_hdf5 = H5_addr_defined(sig_addr);
}

done:
/* Close the file */
if (lf)
if (H5FD_close(lf) < 0 && true == ret_value)
if (H5FD_close(lf) < 0 && true == *is_hdf5)
HDONE_ERROR(H5E_FILE, H5E_CANTCLOSEFILE, FAIL, "unable to close file");

FUNC_LEAVE_NOAPI(ret_value)
Expand Down
2 changes: 1 addition & 1 deletion src/H5Fpkg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ H5_DLLVAR htri_t ignore_disabled_locks_g;
H5_DLL herr_t H5F__post_open(H5F_t *f);
H5_DLL H5F_t *H5F__reopen(H5F_t *f);
H5_DLL herr_t H5F__flush(H5F_t *f);
H5_DLL htri_t H5F__is_hdf5(const char *name, hid_t fapl_id);
H5_DLL herr_t H5F__is_hdf5(const char *name, hid_t fapl_id, bool *is_hdf5);
H5_DLL herr_t H5F__get_file_image(H5F_t *f, void *buf_ptr, size_t buf_len, size_t *image_len);
H5_DLL herr_t H5F__get_info(H5F_t *f, H5F_info2_t *finfo);
H5_DLL herr_t H5F__format_convert(H5F_t *f);
Expand Down
28 changes: 3 additions & 25 deletions src/H5VLcallback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3533,8 +3533,6 @@ H5VL__file_open_find_connector_cb(H5PL_type_t plugin_type, const void *plugin_in
H5P_genplist_t *fapl_plist;
H5P_genplist_t *fapl_plist_copy;
bool is_accessible = false; /* Whether file is accessible */
ssize_t num_errors = 0;
herr_t status;
hid_t connector_id = H5I_INVALID_HID;
hid_t fapl_id = H5I_INVALID_HID;
herr_t ret_value = H5_ITER_CONT;
Expand Down Expand Up @@ -3572,30 +3570,10 @@ H5VL__file_open_find_connector_cb(H5PL_type_t plugin_type, const void *plugin_in
vol_cb_args.args.is_accessible.fapl_id = fapl_id;
vol_cb_args.args.is_accessible.accessible = &is_accessible;

/* 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely, I'll add some blurbs for it in the morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jhendersonHDF - How's this look?

HGOTO_ERROR(H5E_FILE, H5E_CANTGET, H5_ITER_ERROR, "error when checking for accessible HDF5 file");

/* Check if file is accessible with given VOL connector */
H5E_BEGIN_TRY
{
status = H5VL_file_specific(NULL, &vol_cb_args, H5P_DATASET_XFER_DEFAULT, H5_REQUEST_NULL);
}
H5E_END_TRY

if (status < 0) {
ssize_t new_num_errors = 0;

/* Pop any errors generated by the above call */
if ((new_num_errors = H5Eget_num(H5E_DEFAULT)) < 0)
HGOTO_ERROR(H5E_ERROR, H5E_CANTGET, H5_ITER_ERROR, "can't get current error stack size");
if (new_num_errors > num_errors) {
new_num_errors -= num_errors;
if (H5Epop(H5E_DEFAULT, (size_t)new_num_errors) < 0)
HGOTO_ERROR(H5E_ERROR, H5E_CANTRELEASE, H5_ITER_ERROR, "can't sanitize error stack");
}
}
else if (status == SUCCEED && is_accessible) {
if (is_accessible) {
/* If the file was accessible with the current VOL connector, return
* the FAPL with that VOL connector set on it.
*/
Expand Down
8 changes: 1 addition & 7 deletions src/H5VLnative_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,15 +339,9 @@ H5VL__native_file_specific(void *obj, H5VL_file_specific_args_t *args, hid_t H5_

/* H5Fis_accessible */
case H5VL_FILE_IS_ACCESSIBLE: {
htri_t result;

if ((result = H5F__is_hdf5(args->args.is_accessible.filename, args->args.is_accessible.fapl_id)) <
0)
if (H5F__is_hdf5(args->args.is_accessible.filename, args->args.is_accessible.fapl_id, args->args.is_accessible.accessible) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "error in HDF5 file check");

/* Set 'out' value */
*args->args.is_accessible.accessible = (bool)result;

break;
}

Expand Down
Loading