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

Eliminate more H5E_BEGIN/END_TRY macros and H5E_clear_stack() calls #4648

Merged
merged 6 commits into from
Jul 17, 2024
Merged

Eliminate more H5E_BEGIN/END_TRY macros and H5E_clear_stack() calls #4648

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e8bde14
Eliminate more H5E_BEGIN/END_TRY macros and H5E_clear_stack() calls
qkoziol Jul 12, 2024
1e7c9fd
Committing clang-format changes
github-actions[bot] Jul 12, 2024
ea4c86d
Update per reviewer feedback
qkoziol Jul 15, 2024
b2110a3
Committing clang-format changes
github-actions[bot] Jul 15, 2024
a94b0b8
Merge remote-tracking branch 'origin/develop' into pause_errors_5
qkoziol Jul 15, 2024
03a3f5c
Merge remote-tracking branch 'origin/develop' into pause_errors_5
qkoziol Jul 16, 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
79 changes: 41 additions & 38 deletions src/H5FDfamily.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ H5FD__family_fapl_get(H5FD_t *_file)
FUNC_ENTER_PACKAGE

if (NULL == (fa = (H5FD_family_fapl_t *)H5MM_calloc(sizeof(H5FD_family_fapl_t))))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed");
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, NULL, "memory allocation failed");

fa->memb_size = file->memb_size;
if (NULL == (plist = (H5P_genplist_t *)H5I_object(file->memb_fapl_id)))
Expand Down Expand Up @@ -463,7 +463,7 @@ H5FD__family_fapl_copy(const void *_old_fa)
FUNC_ENTER_PACKAGE

if (NULL == (new_fa = (H5FD_family_fapl_t *)H5MM_malloc(sizeof(H5FD_family_fapl_t))))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "memory allocation failed");
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, NULL, "memory allocation failed");

/* Copy the fields of the structure */
H5MM_memcpy(new_fa, old_fa, sizeof(H5FD_family_fapl_t));
Expand Down Expand Up @@ -671,7 +671,7 @@ H5FD__family_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxad

/* Initialize file from file access properties */
if (NULL == (file = (H5FD_family_t *)H5MM_calloc(sizeof(H5FD_family_t))))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "unable to allocate file struct");
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, NULL, "unable to allocate file struct");
if (H5P_FILE_ACCESS_DEFAULT == fapl_id) {
H5FD_family_fapl_t default_fa;

Expand Down Expand Up @@ -760,7 +760,7 @@ H5FD__family_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxad

assert(n > 0);
if (NULL == (x = (H5FD_t **)H5MM_realloc(file->memb, n * sizeof(H5FD_t *))))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL, "unable to reallocate members");
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, NULL, "unable to reallocate members");
file->amembs = n;
file->memb = x;
} /* end if */
Expand All @@ -770,18 +770,23 @@ H5FD__family_open(const char *name, unsigned flags, hid_t fapl_id, haddr_t maxad
* otherwise an open failure means that we've reached the last member.
* Allow H5F_ACC_CREAT only on the first family member.
*/
H5E_BEGIN_TRY
{
file->memb[file->nmembs] =
H5FDopen(memb_name, (0 == file->nmembs ? flags : t_flags), file->memb_fapl_id, HADDR_UNDEF);
if (0 == file->nmembs) {
if (NULL == (file->memb[file->nmembs] = H5FDopen(memb_name, (0 == file->nmembs ? flags : t_flags),
file->memb_fapl_id, HADDR_UNDEF)))
HGOTO_ERROR(H5E_VFL, H5E_CANTOPENFILE, NULL, "unable to open member file");
}
H5E_END_TRY
if (!file->memb[file->nmembs]) {
if (0 == file->nmembs)
HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, NULL, "unable to open member file");
H5E_clear_stack();
break;
else {
H5E_PAUSE_ERRORS
{
file->memb[file->nmembs] = H5FDopen(memb_name, (0 == file->nmembs ? flags : t_flags),
file->memb_fapl_id, HADDR_UNDEF);
}
H5E_RESUME_ERRORS

if (!file->memb[file->nmembs])
break;
}

file->nmembs++;
}

Expand Down Expand Up @@ -1005,7 +1010,7 @@ H5FD__family_set_eoa(H5FD_t *_file, H5FD_mem_t type, haddr_t abs_eoa)
H5FD_t **x = (H5FD_t **)H5MM_realloc(file->memb, n * sizeof(H5FD_t *));

if (!x)
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "unable to allocate memory block");
HGOTO_ERROR(H5E_VFL, H5E_CANTALLOC, FAIL, "unable to allocate memory block");
file->amembs = n;
file->memb = x;
file->nmembs = u;
Expand All @@ -1015,14 +1020,9 @@ H5FD__family_set_eoa(H5FD_t *_file, H5FD_mem_t type, haddr_t abs_eoa)
if (u >= file->nmembs || !file->memb[u]) {
file->nmembs = MAX(file->nmembs, u + 1);
snprintf(memb_name, H5FD_FAM_MEMB_NAME_BUF_SIZE, file->name, u);
H5E_BEGIN_TRY
{
H5_CHECK_OVERFLOW(file->memb_size, hsize_t, haddr_t);
file->memb[u] = H5FDopen(memb_name, file->flags | H5F_ACC_CREAT, file->memb_fapl_id,
(haddr_t)file->memb_size);
}
H5E_END_TRY
if (NULL == file->memb[u])
H5_CHECK_OVERFLOW(file->memb_size, hsize_t, haddr_t);
if (NULL == (file->memb[u] = H5FDopen(memb_name, file->flags | H5F_ACC_CREAT, file->memb_fapl_id,
(haddr_t)file->memb_size)))
HGOTO_ERROR(H5E_FILE, H5E_CANTOPENFILE, FAIL, "unable to open member file");
} /* end if */

Expand Down Expand Up @@ -1082,7 +1082,7 @@ H5FD__family_get_eof(const H5FD_t *_file, H5FD_mem_t type)
* loop with i==0.
*/
assert(file->nmembs > 0);
for (i = (int)file->nmembs - 1; i >= 0; --i) {
for (i = (int)(file->nmembs - 1); i >= 0; --i) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change? It looks like it could create a warning about subtracting a signed number from an unsigned (I assume the (int) was added in the first place to avoid that).

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 actually removed a warning. :-) I can't remember exactly, but one of the ones like "assuming that the range doesn't wrap around".

(It can't actually go negative, since the assert guarantees that nmembs is > 0)

if ((eof = H5FD_get_eof(file->memb[i], type)) != 0)
break;
if (0 == i)
Expand Down Expand Up @@ -1418,10 +1418,9 @@ H5FD__family_delete(const char *filename, hid_t fapl_id)
bool default_config = false;
hid_t memb_fapl_id = H5I_INVALID_HID;
unsigned current_member;
char *member_name = NULL;
char *temp = NULL;
herr_t delete_error = FAIL;
herr_t ret_value = SUCCEED;
char *member_name = NULL;
char *temp = NULL;
herr_t ret_value = SUCCEED;

FUNC_ENTER_PACKAGE

Expand Down Expand Up @@ -1488,18 +1487,22 @@ H5FD__family_delete(const char *filename, hid_t fapl_id)
* Note that this means that any missing files in the family will leave
* undeleted members behind.
*/
H5E_BEGIN_TRY
{
delete_error = H5FD_delete(member_name, memb_fapl_id);
}
H5E_END_TRY
if (FAIL == delete_error) {
if (0 == current_member)
if (0 == current_member) {
if (H5FD_delete(member_name, memb_fapl_id) < 0)
HGOTO_ERROR(H5E_VFL, H5E_CANTDELETEFILE, FAIL, "unable to delete member file");
else
H5E_clear_stack();
break;
}
else {
herr_t delete_error;

H5E_PAUSE_ERRORS
{
delete_error = H5FD_delete(member_name, memb_fapl_id);
}
H5E_RESUME_ERRORS
if (delete_error < 0)
break;
}

current_member++;
} /* end while */

Expand Down
54 changes: 24 additions & 30 deletions src/H5Fsuper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ H5F__super_ext_create(H5F_t *f, H5O_loc_t *ext_ptr)
*/
H5O_loc_reset(ext_ptr);
if (H5O_create(f, (size_t)0, (size_t)1, H5P_GROUP_CREATE_DEFAULT, ext_ptr) < 0)
HGOTO_ERROR(H5E_OHDR, H5E_CANTCREATE, FAIL, "unable to create superblock extension");
HGOTO_ERROR(H5E_FILE, H5E_CANTCREATE, FAIL, "unable to create superblock extension");

/* Record the address of the superblock extension */
f->shared->sblock->ext_addr = ext_ptr->addr;
Expand Down Expand Up @@ -156,7 +156,7 @@ H5F__super_ext_open(H5F_t *f, haddr_t ext_addr, H5O_loc_t *ext_ptr)

/* Open the superblock extension object header */
if (H5O_open(ext_ptr) < 0)
HGOTO_ERROR(H5E_OHDR, H5E_CANTOPENOBJ, FAIL, "unable to open superblock extension");
HGOTO_ERROR(H5E_FILE, H5E_CANTOPENOBJ, FAIL, "unable to open superblock extension");

done:
FUNC_LEAVE_NOAPI(ret_value)
Expand Down Expand Up @@ -311,7 +311,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)
H5P_genplist_t *c_plist; /* File creation property list */
H5FD_t *file; /* File driver pointer */
unsigned sblock_flags = H5AC__NO_FLAGS_SET; /* flags used in superblock unprotect call */
haddr_t super_addr; /* Absolute address of superblock */
haddr_t super_addr = HADDR_UNDEF; /* Absolute address of superblock */
haddr_t eof; /* End of file address */
unsigned rw_flags; /* Read/write permissions for file */
bool skip_eof_check = false; /* Whether to skip checking the EOF value */
Expand Down Expand Up @@ -339,7 +339,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)
/* If we are an MPI application with at least two processes, the
* following superblock signature location optimization is applicable.
*
* Note:: For parallel applications which don't setup for using the
* Note: For parallel applications which don't setup for using the
* HDF5 MPIO driver, we will arrive here with mpi_size == 1.
* This occurs because of the variable initialization (above) and the
* fact that we have skipped actually calling MPI functions to determine
Expand All @@ -361,19 +361,13 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)

/* Search for the file's signature only with rank 0 process */
if (0 == mpi_rank) {
herr_t status;

/* Try detecting file's signature */
/* (Don't leave before Bcast, to avoid hang on error) */
H5E_BEGIN_TRY
H5E_PAUSE_ERRORS
{
status = H5FD_locate_signature(file, &super_addr);
H5FD_locate_signature(file, &super_addr);
Copy link
Member

Choose a reason for hiding this comment

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

This should be fine, but interesting note: I looked at H5FD_locate_signature to make sure it won't update sig_addr on failure, and it looks like it's actually meant to set it to HADDR_UNDEF on failure, but it does so after the call to HGOTO_ERROR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙃

}
H5E_END_TRY

/* Set superblock address to undefined on error */
if (status < 0)
super_addr = HADDR_UNDEF;
H5E_RESUME_ERRORS
} /* end if */

/* Broadcast superblock address to other processes */
Expand Down Expand Up @@ -579,7 +573,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)
/* Check if this private property exists in fapl */
if (H5P_exist_plist(fa_plist, H5F_ACS_SKIP_EOF_CHECK_NAME) > 0)
if (H5P_get(fa_plist, H5F_ACS_SKIP_EOF_CHECK_NAME, &skip_eof_check) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, FAIL, "can't get skip EOF check value");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get skip EOF check value");

if (H5F_INTENT(f) & H5F_ACC_SWMR_READ) {
/*
Expand Down Expand Up @@ -760,7 +754,7 @@ H5F__super_read(H5F_t *f, H5P_genplist_t *fa_plist, bool initial_read)
*/
if (H5P_exist_plist(fa_plist, H5F_ACS_NULL_FSM_ADDR_NAME) > 0)
if (H5P_get(fa_plist, H5F_ACS_NULL_FSM_ADDR_NAME, &f->shared->null_fsm_addr) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, FAIL,
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL,
"can't get clearance for persisting fsm addr");

/* Retrieve the 'file space info' structure */
Expand Down Expand Up @@ -1091,7 +1085,7 @@ H5F__super_init(H5F_t *f)

/* Allocate space for the superblock */
if (NULL == (sblock = H5FL_CALLOC(H5F_super_t)))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed");
HGOTO_ERROR(H5E_FILE, H5E_CANTALLOC, FAIL, "memory allocation failed");

/* Initialize various address information */
sblock->base_addr = HADDR_UNDEF;
Expand All @@ -1101,15 +1095,15 @@ H5F__super_init(H5F_t *f)

/* Get the shared file creation property list */
if (NULL == (plist = (H5P_genplist_t *)H5I_object(f->shared->fcpl_id)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a property list");
HGOTO_ERROR(H5E_FILE, H5E_BADTYPE, FAIL, "not a property list");

/* Initialize sym_leaf_k */
if (H5P_get(plist, H5F_CRT_SYM_LEAF_NAME, &sblock->sym_leaf_k) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, FAIL, "can't get byte number for object size");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get byte number for object size");

/* Initialize btree_k */
if (H5P_get(plist, H5F_CRT_BTREE_RANK_NAME, &sblock->btree_k[0]) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, FAIL, "unable to get rank for btree internal nodes");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to get rank for btree internal nodes");

/* Check for non-default free-space settings */
if (!(f->shared->fs_strategy == H5F_FILE_SPACE_STRATEGY_DEF &&
Expand Down Expand Up @@ -1184,9 +1178,9 @@ H5F__super_init(H5F_t *f)
H5P_genplist_t *c_plist; /* Property list */

if (NULL == (c_plist = (H5P_genplist_t *)H5I_object(f->shared->fcpl_id)))
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not property list");
HGOTO_ERROR(H5E_FILE, H5E_BADTYPE, FAIL, "not property list");
if (H5P_set(c_plist, H5F_CRT_SUPER_VERS_NAME, &super_vers) < 0)
HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, FAIL, "unable to set superblock version");
HGOTO_ERROR(H5E_FILE, H5E_CANTSET, FAIL, "unable to set superblock version");
} /* end if */

if (H5FD_set_paged_aggr(f->shared->lf, (bool)H5F_PAGED_AGGR(f)) < 0)
Expand Down Expand Up @@ -1271,15 +1265,15 @@ H5F__super_init(H5F_t *f)
/* Insert superblock into cache, pinned */
if (H5AC_insert_entry(f, H5AC_SUPERBLOCK, (haddr_t)0, sblock,
H5AC__PIN_ENTRY_FLAG | H5AC__FLUSH_LAST_FLAG | H5AC__FLUSH_COLLECTIVELY_FLAG) < 0)
HGOTO_ERROR(H5E_CACHE, H5E_CANTINS, FAIL, "can't add superblock to cache");
HGOTO_ERROR(H5E_FILE, H5E_CANTINS, FAIL, "can't add superblock to cache");
sblock_in_cache = true;

/* Keep a copy of the superblock info */
f->shared->sblock = sblock;

/* Allocate space for the superblock */
if (HADDR_UNDEF == (superblock_addr = H5MF_alloc(f, H5FD_MEM_SUPER, superblock_size)))
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "file allocation failed for superblock");
HGOTO_ERROR(H5E_FILE, H5E_CANTALLOC, FAIL, "file allocation failed for superblock");

/* set the drvinfo filed to NULL -- will overwrite this later if needed */
f->shared->drvinfo = NULL;
Expand Down Expand Up @@ -1719,15 +1713,15 @@ H5F__super_ext_write_msg(H5F_t *f, unsigned id, void *mesg, bool may_create, uns
/* Check for creating vs. writing */
if (may_create) {
if (status)
HGOTO_ERROR(H5E_OHDR, H5E_CANTGET, FAIL, "Message should not exist");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "Message should not exist");

/* Create the message with ID in the superblock extension */
if (H5O_msg_create(&ext_loc, id, (mesg_flags | H5O_MSG_FLAG_DONTSHARE), H5O_UPDATE_TIME, mesg) < 0)
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to create the message in object header");
} /* end if */
else {
if (!status)
HGOTO_ERROR(H5E_OHDR, H5E_CANTGET, FAIL, "Message should exist");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "Message should exist");

/* Update the message with ID in the superblock extension */
if (H5O_msg_write(&ext_loc, id, (mesg_flags | H5O_MSG_FLAG_DONTSHARE), H5O_UPDATE_TIME, mesg) < 0)
Expand Down Expand Up @@ -1784,27 +1778,27 @@ H5F__super_ext_remove_msg(H5F_t *f, unsigned id)

/* Check if message with ID exists in the object header */
if ((status = H5O_msg_exists(&ext_loc, id)) < 0)
HGOTO_ERROR(H5E_OHDR, H5E_CANTGET, FAIL, "unable to check object header for message");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to check object header for message");
else if (status) {
/* message exists */
H5O_hdr_info_t hdr_info; /* Object header info for superblock extension */

/* Remove the message */
if (H5O_msg_remove(&ext_loc, id, H5O_ALL, true) < 0)
HGOTO_ERROR(H5E_OHDR, H5E_CANTDELETE, FAIL, "unable to delete free-space manager info message");
HGOTO_ERROR(H5E_FILE, H5E_CANTDELETE, FAIL, "unable to delete free-space manager info message");

/* Get info for the superblock extension's object header */
if (H5O_get_hdr_info(&ext_loc, &hdr_info) < 0)
HGOTO_ERROR(H5E_OHDR, H5E_CANTGET, FAIL, "unable to retrieve superblock extension info");
HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to retrieve superblock extension info");

/* If the object header is an empty base chunk, remove superblock extension */
if (hdr_info.nchunks == 1) {
if ((null_count = H5O_msg_count(&ext_loc, H5O_NULL_ID)) < 0)
HGOTO_ERROR(H5E_SYM, H5E_CANTCOUNT, FAIL, "unable to count messages");
HGOTO_ERROR(H5E_FILE, H5E_CANTCOUNT, FAIL, "unable to count messages");
else if ((unsigned)null_count == hdr_info.nmesgs) {
assert(H5_addr_defined(ext_loc.addr));
if (H5O_delete(f, ext_loc.addr) < 0)
HGOTO_ERROR(H5E_SYM, H5E_CANTCOUNT, FAIL, "unable to count messages");
HGOTO_ERROR(H5E_FILE, H5E_CANTCOUNT, FAIL, "unable to count messages");
f->shared->sblock->ext_addr = HADDR_UNDEF;
} /* end else-if */
} /* end if */
Expand Down
Loading