diff options
author | Richard Warren <Richard.Warren@hdfgroup.org> | 2017-10-11 17:40:54 (GMT) |
---|---|---|
committer | Richard Warren <Richard.Warren@hdfgroup.org> | 2017-10-11 17:40:54 (GMT) |
commit | 157398107e334e3dafbdcd25f34da391510e45f2 (patch) | |
tree | 41ba7c06b2b2fe645f6df846e0c3239bf63df7ee | |
parent | 9849d61344faf01f366fff2e67026e468e184f06 (diff) | |
download | hdf5-157398107e334e3dafbdcd25f34da391510e45f2.zip hdf5-157398107e334e3dafbdcd25f34da391510e45f2.tar.gz hdf5-157398107e334e3dafbdcd25f34da391510e45f2.tar.bz2 |
Try to address most of the issues raised by Dana in the code review
-rw-r--r-- | src/H5Fsuper.c | 102 | ||||
-rw-r--r-- | testpar/t_pread.c | 104 |
2 files changed, 101 insertions, 105 deletions
diff --git a/src/H5Fsuper.c b/src/H5Fsuper.c index a3b1fed..0c6f9cd 100644 --- a/src/H5Fsuper.c +++ b/src/H5Fsuper.c @@ -21,15 +21,15 @@ /***********/ /* Headers */ /***********/ -#include "H5private.h" /* Generic Functions */ +#include "H5private.h" /* Generic Functions */ #include "H5ACprivate.h" /* Metadata cache */ -#include "H5Eprivate.h" /* Error handling */ +#include "H5Eprivate.h" /* Error handling */ #include "H5Fpkg.h" /* File access */ -#include "H5FDprivate.h" /* File drivers */ -#include "H5Iprivate.h" /* IDs */ +#include "H5FDprivate.h" /* File drivers */ +#include "H5Iprivate.h" /* IDs */ #include "H5MFprivate.h" /* File memory management */ -#include "H5MMprivate.h" /* Memory management */ -#include "H5Pprivate.h" /* Property lists */ +#include "H5MMprivate.h" /* Memory management */ +#include "H5Pprivate.h" /* Property lists */ #include "H5SMprivate.h" /* Shared Object Header Messages */ @@ -158,7 +158,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_OHDR, H5E_CANTOPENOBJ, FAIL, "unable to open superblock extension") done: FUNC_LEAVE_NOAPI(ret_value) @@ -224,12 +224,12 @@ done: /*------------------------------------------------------------------------- * Function: H5F__update_super_ext_driver_msg * - * Purpose: Update the superblock extension file driver info message if - * we are using a V 2 superblock. Observe that the function - * is a NO-OP if the file driver info message does not exist. + * Purpose: Update the superblock extension file driver info message if + * we are using a V 2 superblock. Observe that the function + * is a NO-OP if the file driver info message does not exist. * This is necessary, as the function is called whenever the - * EOA is updated, and were it to create the file driver info - * message, it would find itself in an infinite recursion. + * EOA is updated, and were it to create the file driver info + * message, it would find itself in an infinite recursion. * * Return: Success: SUCCEED * Failure: FAIL @@ -267,7 +267,7 @@ H5F__update_super_ext_driver_msg(H5F_t *f, hid_t dxpl_id) /* Check for driver info */ H5_CHECKED_ASSIGN(driver_size, size_t, H5FD_sb_size(f->shared->lf), hsize_t); - /* Nothing to do unless there is both driver info and + /* Nothing to do unless there is both driver info and * the driver info superblock extension message has * already been created. */ @@ -330,13 +330,13 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial unsigned sblock_flags = H5AC__NO_FLAGS_SET; /* flags used in superblock unprotect call */ haddr_t super_addr; /* Absolute address of superblock */ haddr_t eof; /* End of file address */ - unsigned rw_flags; /* Read/write permissions for file */ - hbool_t skip_eof_check = FALSE; /* Whether to skip checking the EOF value */ + unsigned rw_flags; /* Read/write permissions for file */ + hbool_t skip_eof_check = FALSE; /* Whether to skip checking the EOF value */ herr_t ret_value = SUCCEED; /* Return value */ #ifdef H5_HAVE_PARALLEL int mpi_rank = 0, mpi_size = 1; int mpi_result; -#endif /* H5_HAVE_PARALLEL */ +#endif /* H5_HAVE_PARALLEL */ FUNC_ENTER_PACKAGE_TAG(meta_dxpl_id, H5AC__SUPERBLOCK_TAG, FAIL) @@ -365,7 +365,7 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "Can't get MPI rank") if((mpi_size = H5F_mpi_get_size(f)) < 0) - HGOTO_ERROR(H5E_PAGEBUF, H5E_CANTGET, FAIL, "can't retrieve MPI communicator size") + HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't retrieve MPI communicator size") } /* If we are an MPI application with at least two processes, the @@ -380,14 +380,14 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial if ( mpi_size > 1 ) { MPI_Comm this_comm = MPI_COMM_NULL; - if ( mpi_rank == 0 ) { - if(H5FD_locate_signature(&fdio_info, &super_addr) < 0) - HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "unable to locate file signature") + if ( mpi_rank == 0 ) { + if (H5FD_locate_signature(&fdio_info, &super_addr) < 0) + HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "unable to locate file signature") } HDassert(H5F_HAS_FEATURE(f, H5FD_FEAT_HAS_MPI)); if ( MPI_COMM_NULL == (this_comm = H5F_mpi_get_comm(f)) ) - HGOTO_ERROR(H5E_VFL, H5E_CANTGET, FAIL, "can't get MPI communicator") + HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't get MPI communicator") if ( MPI_SUCCESS != (mpi_result = MPI_Bcast(&super_addr,sizeof(super_addr), MPI_BYTE, 0, this_comm))) @@ -395,14 +395,14 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial } else { /* Locate the signature as per per the serial library */ -#endif /* H5_HAVE_PARALLEL */ +#endif /* H5_HAVE_PARALLEL */ - if(H5FD_locate_signature(&fdio_info, &super_addr) < 0) - HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "unable to locate file signature") + if (H5FD_locate_signature(&fdio_info, &super_addr) < 0) + HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "unable to locate file signature") #ifdef H5_HAVE_PARALLEL } -#endif /* H5_HAVE_PARALLEL */ +#endif /* H5_HAVE_PARALLEL */ if(HADDR_UNDEF == super_addr) HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "file signature not found") @@ -453,12 +453,12 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial HGOTO_ERROR(H5E_FILE, H5E_CANTPROTECT, FAIL, "unable to load superblock") if(H5F_INTENT(f) & H5F_ACC_SWMR_WRITE) - if(sblock->super_vers < HDF5_SUPERBLOCK_VERSION_3) - HGOTO_ERROR(H5E_FILE, H5E_CANTPROTECT, FAIL, "invalid superblock version for SWMR_WRITE") + if(sblock->super_vers < HDF5_SUPERBLOCK_VERSION_3) + HGOTO_ERROR(H5E_FILE, H5E_CANTPROTECT, FAIL, "invalid superblock version for SWMR_WRITE") /* Enable all latest version support when file has v3 superblock */ if(sblock->super_vers >= HDF5_SUPERBLOCK_VERSION_3) - f->shared->latest_flags |= H5F_LATEST_ALL_FLAGS; + f->shared->latest_flags |= H5F_LATEST_ALL_FLAGS; /* Pin the superblock in the cache */ if(H5AC_pin_protected_entry(sblock) < 0) @@ -558,15 +558,15 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial * been flushed to disk by the SWMR writer process. */ if(H5F_INTENT(f) & H5F_ACC_SWMR_READ) { - /* - * When the file is opened for SWMR read access, skip the check if: - * --the file is already marked for SWMR writing and - * --the file has version 3 superblock for SWMR support - */ - if((sblock->status_flags & H5F_SUPER_SWMR_WRITE_ACCESS) && + /* + * When the file is opened for SWMR read access, skip the check if: + * --the file is already marked for SWMR writing and + * --the file has version 3 superblock for SWMR support + */ + if((sblock->status_flags & H5F_SUPER_SWMR_WRITE_ACCESS) && (sblock->status_flags & H5F_SUPER_WRITE_ACCESS) && sblock->super_vers >= HDF5_SUPERBLOCK_VERSION_3) - skip_eof_check = TRUE; + skip_eof_check = TRUE; } /* end if */ if(!skip_eof_check && initial_read) { if(HADDR_UNDEF == (eof = H5FD_get_eof(f->shared->lf, H5FD_MEM_DEFAULT))) @@ -640,7 +640,7 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial H5O_loc_t ext_loc; /* "Object location" for superblock extension */ H5O_btreek_t btreek; /* v1 B-tree 'K' value message from superblock extension */ H5O_drvinfo_t drvinfo; /* Driver info message from superblock extension */ - size_t u; /* Local index variable */ + size_t u; /* Local index variable */ htri_t status; /* Status for message existing */ /* Sanity check - superblock extension should only be defined for @@ -661,7 +661,7 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial } /* end if */ /* Open the superblock extension */ - if(H5F_super_ext_open(f, sblock->ext_addr, &ext_loc) < 0) + if(H5F_super_ext_open(f, sblock->ext_addr, &ext_loc) < 0) HGOTO_ERROR(H5E_FILE, H5E_CANTOPENOBJ, FAIL, "unable to open file's superblock extension") /* Check for the extension having a 'driver info' message */ @@ -684,8 +684,8 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial /* Reset driver info message */ H5O_msg_reset(H5O_DRVINFO_ID, &drvinfo); - HDassert(FALSE == f->shared->drvinfo_sb_msg_exists); - f->shared->drvinfo_sb_msg_exists = TRUE; + HDassert(FALSE == f->shared->drvinfo_sb_msg_exists); + f->shared->drvinfo_sb_msg_exists = TRUE; } /* end else */ } /* end if */ @@ -811,37 +811,37 @@ H5F__super_read(H5F_t *f, hid_t meta_dxpl_id, hid_t raw_dxpl_id, hbool_t initial } /* end if not marked "unknown" */ } /* end if */ - /* Check for the extension having a 'metadata cache image' message */ + /* Check for the extension having a 'metadata cache image' message */ if((status = H5O_msg_exists(&ext_loc, H5O_MDCI_MSG_ID, meta_dxpl_id)) < 0) HGOTO_ERROR(H5E_FILE, H5E_EXISTS, FAIL, "unable to read object header") if(status) { - hbool_t rw = ((rw_flags & H5AC__READ_ONLY_FLAG) == 0); - H5O_mdci_t mdci_msg; + hbool_t rw = ((rw_flags & H5AC__READ_ONLY_FLAG) == 0); + H5O_mdci_t mdci_msg; - /* if the metadata cache image superblock extension message exists, + /* if the metadata cache image superblock extension message exists, * read its contents and pass the data on to the metadata cache. * Given this data, the cache will load and decode the metadata - * cache image block, decoded it and load its contents into the - * the cache on the test protect call. + * cache image block, decoded it and load its contents into the + * the cache on the test protect call. * * Further, if the file is opened R/W, the metadata cache will - * delete the metadata cache image superblock extension and free - * the cache image block. Don't do this now as f->shared - * is not fully setup, which complicates matters. + * delete the metadata cache image superblock extension and free + * the cache image block. Don't do this now as f->shared + * is not fully setup, which complicates matters. */ /* Retrieve the 'metadata cache image message' structure */ - if(NULL == H5O_msg_read(&ext_loc, H5O_MDCI_MSG_ID, &mdci_msg, meta_dxpl_id)) + if(NULL == H5O_msg_read(&ext_loc, H5O_MDCI_MSG_ID, &mdci_msg, meta_dxpl_id)) HGOTO_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "unable to get metadata cache image message") /* Indicate to the cache that there's an image to load on first protect call */ if(H5AC_load_cache_image_on_next_protect(f, mdci_msg.addr, mdci_msg.size, rw) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTLOAD, FAIL, "call to H5AC_load_cache_image_on_next_protect failed"); + HGOTO_ERROR(H5E_FILE, H5E_CANTLOAD, FAIL, "call to H5AC_load_cache_image_on_next_protect failed"); } /* end if */ /* Close superblock extension */ if(H5F_super_ext_close(f, &ext_loc, meta_dxpl_id, FALSE) < 0) - HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEOBJ, FAIL, "unable to close file's superblock extension") + HGOTO_ERROR(H5E_FILE, H5E_CANTCLOSEOBJ, FAIL, "unable to close file's superblock extension") } /* end if */ /* Update the driver info if VFD indicated to do so */ diff --git a/testpar/t_pread.c b/testpar/t_pread.c index b40fc09..f0cad3d 100644 --- a/testpar/t_pread.c +++ b/testpar/t_pread.c @@ -87,17 +87,13 @@ static int test_parallel_read(MPI_Comm comm, int mpi_rank, int group); * * Failure: 1 * - * Programmer: Richard Warren - * 10/1/17 - * - * Modifications: * *------------------------------------------------------------------------- */ static int generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) { - FILE *header; + FILE *header = NULL; const char *fcn_name = "generate_test_file()"; const char *failure_mssg = NULL; const char *group_filename = NULL; @@ -113,12 +109,12 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) hsize_t i; hsize_t offset; hsize_t dims[1] = {0}; - hid_t file_id; - hid_t memspace; - hid_t filespace; - hid_t fapl_id; - hid_t dxpl_id; - hid_t dset_id; + hid_t file_id = -1; + hid_t memspace = -1; + hid_t filespace = -1; + hid_t fapl_id = -1; + hid_t dxpl_id = -1; + hid_t dset_id = -1; float nextValue; float *data_slice = NULL; @@ -155,17 +151,17 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) else if ( group_id == 0 ) { /* Test 2 group 0 */ file_index = 3; } - else { /* Test 2 group 1 */ + else { /* Test 2 group 1 */ file_index = 6; } - /* The 'group_filename' is just a temp variable and + /* The 'group_filename' is just a temp variable and * is used to call into the h5_fixname function. No * need to worry that we reassign it for each file! - */ + */ HDassert((group_filename = FILENAMES[file_index])); - /* Assign the 'data_filename' */ + /* Assign the 'data_filename' */ if ( h5_fixname(group_filename, H5P_DEFAULT, data_filename, sizeof(data_filename)) == NULL ) { pass = FALSE; @@ -177,7 +173,7 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) HDassert( (group_filename = FILENAMES[file_index+1]) ); - /* Assign the 'reloc_data_filename' */ + /* Assign the 'reloc_data_filename' */ if ( h5_fixname(group_filename, H5P_DEFAULT, reloc_data_filename, sizeof(reloc_data_filename)) == NULL ) { @@ -190,7 +186,7 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) HDassert( (group_filename = FILENAMES[file_index+2]) ); - /* Assing the 'prolog_filename' */ + /* Assign the 'prolog_filename' */ if ( h5_fixname(group_filename, H5P_DEFAULT, prolog_filename, sizeof(prolog_filename)) == NULL ) { pass = FALSE; @@ -211,7 +207,7 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) for(i=0; i<COUNT; i++) { data_slice[i] = nextValue; - nextValue += 1; + nextValue += 1; } } @@ -297,42 +293,42 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) } /* close file, etc. */ - if ( pass ) { + if ( pass || (dset_id != -1)) { if ( H5Dclose(dset_id) < 0 ) { pass = false; failure_mssg = "H5Dclose(dset_id) failed.\n"; } } - if ( pass ) { + if ( pass || (memspace != -1) ) { if ( H5Sclose(memspace) < 0 ) { pass = false; failure_mssg = "H5Sclose(memspace) failed.\n"; } } - if ( pass ) { + if ( pass || (filespace != -1) ) { if ( H5Sclose(filespace) < 0 ) { pass = false; failure_mssg = "H5Sclose(filespace) failed.\n"; } } - if ( pass ) { + if ( pass || (file_id != -1) ) { if ( H5Fclose(file_id) < 0 ) { pass = false; failure_mssg = "H5Fclose(file_id) failed.\n"; } } - if ( pass ) { + if ( pass || (dxpl_id != -1) ) { if ( H5Pclose(dxpl_id) < 0 ) { pass = false; failure_mssg = "H5Pclose(dxpl_id) failed.\n"; } } - if ( pass ) { + if ( pass || (fapl_id != -1) ) { if ( H5Pclose(fapl_id) < 0 ) { pass = false; failure_mssg = "H5Pclose(fapl_id) failed.\n"; @@ -361,10 +357,10 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) if (group_id == 0) text_to_write = random_hdf5_text; - else + else text_to_write = hitchhiker_quote; - bytes_to_write = strlen(text_to_write); + bytes_to_write = strlen(text_to_write); if ( pass ) { if ( (header = HDfopen(prolog_filename, "w+")) == NULL ) { @@ -382,7 +378,7 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) } } - if ( pass ) { + if ( pass || (header != NULL) ) { if ( HDfclose(header) != 0 ) { pass = FALSE; failure_mssg = "HDfclose() failed.\n"; @@ -395,16 +391,14 @@ generate_test_file( MPI_Comm comm, int mpi_rank, int group_id ) HDsprintf(cmd, "../tools/src/h5jam/h5jam -i %s -u %s -o %s", data_filename, prolog_filename, reloc_data_filename); - if ( system(cmd) != 0 ) { + if ( system(cmd) != 0 ) { pass = FALSE; failure_mssg = "invocation of h5jam failed.\n"; } } - if ( pass ) { - HDremove(prolog_filename); - HDremove(data_filename); - } + HDremove(prolog_filename); + HDremove(data_filename); } /* collect results from other processes. @@ -492,10 +486,10 @@ test_parallel_read(MPI_Comm comm, int mpi_rank, int group_id) int global_failures = 0; int group_size; int group_rank; - hid_t fapl_id; - hid_t file_id; - hid_t dset_id; - hid_t memspace = -1; + hid_t fapl_id = -1; + hid_t file_id = -1; + hid_t dset_id = -1; + hid_t memspace = -1; hid_t filespace = -1; hsize_t i; hsize_t offset; @@ -542,11 +536,11 @@ test_parallel_read(MPI_Comm comm, int mpi_rank, int group_id) */ if ( pass ) { - if ( comm == MPI_COMM_WORLD ) /* test 1 */ + if ( comm == MPI_COMM_WORLD ) /* test 1 */ group_filename = FILENAMES[1]; - else if ( group_id == 0 ) /* test 2 group 0 */ + else if ( group_id == 0 ) /* test 2 group 0 */ group_filename = FILENAMES[4]; - else /* test 2 group 1 */ + else /* test 2 group 1 */ group_filename = FILENAMES[7]; HDassert(group_filename); @@ -645,35 +639,35 @@ test_parallel_read(MPI_Comm comm, int mpi_rank, int group_id) } /* close file, etc. */ - if ( pass ) { + if ( pass || (dset_id != -1) ) { if ( H5Dclose(dset_id) < 0 ) { pass = false; failure_mssg = "H5Dclose(dset_id) failed.\n"; } } - if ( pass ) { + if ( pass || (memspace != -1) ) { if ( H5Sclose(memspace) < 0 ) { pass = false; failure_mssg = "H5Sclose(memspace) failed.\n"; } } - if ( pass ) { + if ( pass || (filespace != -1) ) { if ( H5Sclose(filespace) < 0 ) { pass = false; failure_mssg = "H5Sclose(filespace) failed.\n"; } } - if ( pass ) { + if ( pass || (file_id != -1) ) { if ( H5Fclose(file_id) < 0 ) { pass = false; failure_mssg = "H5Fclose(file_id) failed.\n"; } } - if ( pass ) { + if ( pass || (fapl_id != -1) ) { if ( H5Pclose(fapl_id) < 0 ) { pass = false; failure_mssg = "H5Pclose(fapl_id) failed.\n"; @@ -764,17 +758,17 @@ main( int argc, char **argv) if ( (MPI_Init(&argc, &argv)) != MPI_SUCCESS) { HDfprintf(stderr, "FATAL: Unable to initialize MPI\n"); - exit(1); + HDexit(FAIL); } if ( (MPI_Comm_rank(MPI_COMM_WORLD, &mpi_rank)) != MPI_SUCCESS) { HDfprintf(stderr, "FATAL: MPI_Comm_rank returned an error\n"); - exit(2); + HDexit(FAIL); } if ( (MPI_Comm_size(MPI_COMM_WORLD, &mpi_size)) != MPI_SUCCESS) { HDfprintf(stderr, "FATAL: MPI_Comm_size returned an error\n"); - exit(2); + HDexit(FAIL); } H5open(); @@ -835,12 +829,12 @@ main( int argc, char **argv) which_group = (mpi_rank < split_size ? 0 : 1); if ( (MPI_Comm_split(MPI_COMM_WORLD, - which_group, - 0, - &group_comm)) != MPI_SUCCESS) { + which_group, + 0, + &group_comm)) != MPI_SUCCESS) { HDfprintf(stderr, "FATAL: MPI_Comm_split returned an error\n"); - exit(2); + HDexit(FAIL); } nerrs += generate_test_file( group_comm, mpi_rank, which_group ); @@ -870,7 +864,7 @@ main( int argc, char **argv) finish: if ((group_comm != MPI_COMM_NULL) && - (MPI_Comm_free(&group_comm)) != MPI_SUCCESS) { + (MPI_Comm_free(&group_comm)) != MPI_SUCCESS) { HDfprintf(stderr, "MPI_Comm_free failed!\n"); } @@ -895,12 +889,14 @@ finish: } /* close HDF5 library */ - H5close(); + if (H5close() != SUCCEED) { + HDfprintf(stdout, "H5close() failed. (Ignoring)\n"); + } /* MPI_Finalize must be called AFTER H5close which may use MPI calls */ MPI_Finalize(); /* cannot just return (nerrs) because exit code is limited to 1byte */ - return(nerrs > 0); + return((nerrs > 0) ? FAIL : SUCCEED ); } /* main() */ |