From b5b99a5b243cd578b0f880984a81bd41d2965bce Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 19 Mar 2021 09:03:05 -0700 Subject: [PATCH 1/7] File locks now work on Windows Uses LockFileEx() and UnlockFileEx(). Fixes HDFFV-10191 (partial). --- release_docs/RELEASE.txt | 17 ++++++++ src/H5Fint.c | 9 ++++- src/H5system.c | 47 +++++++++++---------- tools/src/h5repack/h5repack_copy.c | 65 +++++++++++++++++++++--------- 4 files changed, 95 insertions(+), 43 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 0d3c2a5dd1a..9b21139f149 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -384,6 +384,23 @@ New Features Library: -------- + - File locking now works on Windows + + Since version 1.10.0, the HDF5 library has used a file locking scheme + to help enforce one reader at a time accessing an HDF5 file, which can + be helpful when setting up readers and writers to use the single- + writer/multiple-readers (SWMR) access pattern. + + In the past, this was only functional on POSIX systems where flock() or + fcntl() were present. Windows used a no-op stub that always succeeded. + + HDF5 now uses LockFileEx() and UnlockFileEx() to lock the file using the + same scheme as POSIX systems. We lock the entire file when we set up the + locks (by passing DWORDMAX as both size parameters to LockFileEx()). + + (DER - 2021/03/19, HDFFV-10191) + + - H5Epush_ret() now requires a trailing semicolon H5Epush_ret() is a function-like macro that has been changed to diff --git a/src/H5Fint.c b/src/H5Fint.c index 920bf510db8..4f3a35e0f43 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -3718,10 +3718,17 @@ H5F__start_swmr_write(H5F_t *f) setup = TRUE; /* Place an advisory lock on the file */ - if (H5F_USE_FILE_LOCKING(f)) + if (H5F_USE_FILE_LOCKING(f)) { + /* Have to unlock on Windows as Win32 doesn't support changing the lock + * type (exclusive vs shared) with a second call. + */ + if (H5FD_unlock(f->shared->lf) < 0) { + HGOTO_ERROR(H5E_FILE, H5E_CANTUNLOCKFILE, FAIL, "unable to unlock the file") + } if (H5FD_lock(f->shared->lf, TRUE) < 0) { HGOTO_ERROR(H5E_FILE, H5E_CANTLOCKFILE, FAIL, "unable to lock the file") } + } /* Mark superblock as dirty */ if (H5F_super_dirty(f) < 0) diff --git a/src/H5system.c b/src/H5system.c index 53307a8c1de..fb7deb0e968 100644 --- a/src/H5system.c +++ b/src/H5system.c @@ -618,40 +618,43 @@ c99_vsnprintf(char *str, size_t size, const char *format, va_list ap) *------------------------------------------------------------------------- */ int -Wflock(int H5_ATTR_UNUSED fd, int H5_ATTR_UNUSED operation) +Wflock(int fd, int operation) { -/* This is a no-op while we implement a Win32 VFD */ -#if 0 -int -Wflock(int fd, int operation) { - - HANDLE hFile; - DWORD dwFlags = LOCKFILE_FAIL_IMMEDIATELY; - DWORD dwReserved = 0; - /* MAXDWORD for entire file */ - DWORD nNumberOfBytesToLockLow = MAXDWORD; - DWORD nNumberOfBytesToLockHigh = MAXDWORD; - /* Must initialize OVERLAPPED struct */ - OVERLAPPED overlapped = {0}; + HANDLE hFile; + DWORD dwFlags = LOCKFILE_FAIL_IMMEDIATELY; + DWORD dwReserved = 0; + /* MAXDWORD locks the entire file */ + DWORD nNumberOfBytesToLockLow = MAXDWORD; + DWORD nNumberOfBytesToLockHigh = MAXDWORD; + /* Must initialize OVERLAPPED struct */ + OVERLAPPED overlapped = {0}; /* Get Windows HANDLE */ - hFile = _get_osfhandle(fd); + if (INVALID_HANDLE_VALUE == (hFile = (HANDLE)_get_osfhandle(fd))) + return -1; /* Convert to Windows flags */ if(operation & LOCK_EX) dwFlags |= LOCKFILE_EXCLUSIVE_LOCK; /* Lock or unlock */ - if(operation & LOCK_UN) - if(0 == UnlockFileEx(hFile, dwReserved, nNumberOfBytesToLockLow, - nNumberOfBytesToLockHigh, &overlapped)) - return -1; - else + if (operation & LOCK_UN) { + if (0 == + UnlockFileEx(hFile, dwReserved, nNumberOfBytesToLockLow, nNumberOfBytesToLockHigh, &overlapped)) { + /* Attempting to unlock an already unlocked file will fail and this can happen + * in H5Fstart_swmr_write(). For now, just ignore the "error" (error code: 0x9e / 158). + */ + if (GetLastError() != 158) + return -1; + } + } + else { if(0 == LockFileEx(hFile, dwFlags, dwReserved, nNumberOfBytesToLockLow, - nNumberOfBytesToLockHigh, &overlapped)) + nNumberOfBytesToLockHigh, &overlapped)) return -1; -#endif /* 0 */ + } + return 0; } /* end Wflock() */ diff --git a/tools/src/h5repack/h5repack_copy.c b/tools/src/h5repack/h5repack_copy.c index 7e05a5afea1..757d4989ed8 100644 --- a/tools/src/h5repack/h5repack_copy.c +++ b/tools/src/h5repack/h5repack_copy.c @@ -304,14 +304,6 @@ copy_objects(const char *fnamein, const char *fnameout, pack_opt_t *options) if ((fidout = H5Fcreate(fnameout, H5F_ACC_TRUNC, fcpl, options->fout_fapl)) < 0) H5TOOLS_GOTO_ERROR((-1), "H5Fcreate could not create file <%s>:", fnameout); - /*------------------------------------------------------------------------- - * write a new user block if requested - *------------------------------------------------------------------------- - */ - if (options->ublock_size > 0) - if (copy_user_block(options->ublock_filename, fnameout, options->ublock_size) < 0) - H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting..."); - /*------------------------------------------------------------------------- * get list of objects *------------------------------------------------------------------------- @@ -346,27 +338,60 @@ copy_objects(const char *fnamein, const char *fnameout, pack_opt_t *options) } /*------------------------------------------------------------------------- + * Close the file and everything in it so the lock is removed + *------------------------------------------------------------------------- + */ + if (H5Pclose(fcpl) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + if (H5Pclose(options->fout_fapl) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + options->fout_fapl = H5P_DEFAULT; + if (H5Pclose(gcpl_in) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + if (H5Gclose(grp_in) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + if (H5Fclose(fidout) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + if (H5Fclose(fidin) < 0) + H5TOOLS_GOTO_ERROR((-1), "could not close fcpl"); + + /*------------------------------------------------------------------------- + * NOTE: The userblock MUST be written out AFTER the file is closed or + * the file locking will cause failures on Windows, where file locks + * are per-HANDLE and mandatory, not per-process and advisory. + *------------------------------------------------------------------------- + */ + + /*------------------------------------------------------------------------- + * write a new user block if requested * write only the input file user block if there is no user block file input *------------------------------------------------------------------------- */ - if (ub_size > 0 && options->ublock_size == 0) + if (options->ublock_size > 0) { + if (copy_user_block(options->ublock_filename, fnameout, options->ublock_size) < 0) + H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting..."); + } + else if (ub_size > 0 && options->ublock_size == 0) { if (copy_user_block(fnamein, fnameout, ub_size) < 0) H5TOOLS_GOTO_ERROR((-1), "Could not copy user block. Exiting..."); + } done: - H5E_BEGIN_TRY - { - H5Pclose(fcpl); - H5Pclose(options->fout_fapl); - options->fout_fapl = H5P_DEFAULT; - H5Pclose(gcpl_in); - H5Gclose(grp_in); - H5Pclose(fcpl_in); - H5Fclose(fidout); - H5Fclose(fidin); + if (-1 == ret_value) { + H5E_BEGIN_TRY + { + H5Pclose(fcpl); + H5Pclose(options->fout_fapl); + options->fout_fapl = H5P_DEFAULT; + H5Pclose(gcpl_in); + H5Gclose(grp_in); + H5Pclose(fcpl_in); + H5Fclose(fidout); + H5Fclose(fidin); + } + H5E_END_TRY; } - H5E_END_TRY; if (travt) trav_table_free(travt); From b76b4068256b7ae0679ae9c9d6451a76217a22dc Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 19 Mar 2021 16:06:00 +0000 Subject: [PATCH 2/7] Committing clang-format changes --- src/H5system.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/H5system.c b/src/H5system.c index fb7deb0e968..1ec309e5094 100644 --- a/src/H5system.c +++ b/src/H5system.c @@ -635,7 +635,7 @@ Wflock(int fd, int operation) return -1; /* Convert to Windows flags */ - if(operation & LOCK_EX) + if (operation & LOCK_EX) dwFlags |= LOCKFILE_EXCLUSIVE_LOCK; /* Lock or unlock */ @@ -650,8 +650,8 @@ Wflock(int fd, int operation) } } else { - if(0 == LockFileEx(hFile, dwFlags, dwReserved, nNumberOfBytesToLockLow, - nNumberOfBytesToLockHigh, &overlapped)) + if (0 == LockFileEx(hFile, dwFlags, dwReserved, nNumberOfBytesToLockLow, nNumberOfBytesToLockHigh, + &overlapped)) return -1; } From ba9deaeb66ac820f079702fc298c3aea91793a0f Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 19 Mar 2021 17:04:16 +0000 Subject: [PATCH 3/7] Committing clang-format changes --- src/H5Tconv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index f2dff0befbe..0c986331c2c 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -3843,7 +3843,7 @@ H5T__conv_i_i(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t half_size; /*half the type size */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ size_t first; ssize_t sfirst; /*a signed version of `first' */ @@ -4286,7 +4286,7 @@ H5T__conv_f_f(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t olap; /*num overlapping elements */ ssize_t bitno = 0; /*bit number */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ @@ -8401,7 +8401,7 @@ H5T__conv_f_i(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t tsize; /*type size for swapping bytes */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ @@ -9027,7 +9027,7 @@ H5T__conv_i_f(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t tsize; /*type size for swapping bytes */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ From f525a93b3a738453600817c71be2d904ec23fd84 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Sun, 21 Mar 2021 11:37:15 -0700 Subject: [PATCH 4/7] Fixes commenting in h5repack --- tools/src/h5repack/h5repack_copy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/src/h5repack/h5repack_copy.c b/tools/src/h5repack/h5repack_copy.c index 757d4989ed8..536de69bdeb 100644 --- a/tools/src/h5repack/h5repack_copy.c +++ b/tools/src/h5repack/h5repack_copy.c @@ -358,13 +358,13 @@ copy_objects(const char *fnamein, const char *fnameout, pack_opt_t *options) /*------------------------------------------------------------------------- * NOTE: The userblock MUST be written out AFTER the file is closed or * the file locking will cause failures on Windows, where file locks - * are per-HANDLE and mandatory, not per-process and advisory. + * are mandatory, not advisory. *------------------------------------------------------------------------- */ /*------------------------------------------------------------------------- - * write a new user block if requested - * write only the input file user block if there is no user block file input + * Write a new user block if requested, using the input file user block if + * there is no separate user block file input *------------------------------------------------------------------------- */ From ecd01b7a4a756c56a4b6dec4e02cf8fa0228bf80 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Sun, 21 Mar 2021 16:58:26 -0700 Subject: [PATCH 5/7] Reworks H5Fis_accessible() H5Fis_accessible() created a new file handle and attempted to read through it, which will fail when a file has been opened with an exclusive lock on operating systems that have mandatory locks. This change uses the same scheme we use in H5Fopen() to check if the file is already open and only tries to read the file signature if the file has not already been opened. Also adds a test for this behavior. --- src/H5Fint.c | 25 ++++++++++++++++++------- test/tfile.c | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/H5Fint.c b/src/H5Fint.c index 4f3a35e0f43..4a9c97f7bae 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1056,9 +1056,10 @@ H5F_prefix_open_file(H5F_t *primary_file, H5F_prefix_open_t prefix_type, const c htri_t H5F__is_hdf5(const char *name, hid_t fapl_id) { - H5FD_t *file = NULL; /* Low-level file struct */ - haddr_t sig_addr = HADDR_UNDEF; /* Addess of hdf5 file signature */ - htri_t ret_value = FAIL; /* Return value */ + H5FD_t *file = NULL; /* Low-level file struct */ + H5F_shared_t *shared = NULL; /* Shared part of file */ + haddr_t sig_addr = HADDR_UNDEF; /* Addess of hdf5 file signature */ + htri_t ret_value = FAIL; /* Return value */ FUNC_ENTER_PACKAGE @@ -1069,10 +1070,20 @@ H5F__is_hdf5(const char *name, hid_t fapl_id) if (NULL == (file = H5FD_open(name, H5F_ACC_RDONLY, fapl_id, HADDR_UNDEF))) HGOTO_ERROR(H5E_FILE, H5E_CANTINIT, FAIL, "unable to open file") - /* The file is an hdf5 file if the hdf5 file signature can be found */ - if (H5FD_locate_signature(file, &sig_addr) < 0) - HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "error while trying to locate file signature") - ret_value = (HADDR_UNDEF != sig_addr); + /* If the file is already open, it's an HDF5 file + * + * If the file is open with an exclusive lock on an operating system that enforces + * mandatory file locks (like Windows), creating a new file handle and attempting + * to read through it will fail so we have to try this first. + */ + if ((shared = H5F__sfile_search(file)) != NULL) + ret_value = TRUE; + else { + /* The file is an HDF5 file if the HDF5 file signature can be found */ + if (H5FD_locate_signature(file, &sig_addr) < 0) + HGOTO_ERROR(H5E_FILE, H5E_NOTHDF5, FAIL, "error while trying to locate file signature") + ret_value = (HADDR_UNDEF != sig_addr); + } done: /* Close the file */ diff --git a/test/tfile.c b/test/tfile.c index c8c123c23f6..0d2926ff7b1 100644 --- a/test/tfile.c +++ b/test/tfile.c @@ -1666,6 +1666,29 @@ test_file_is_accessible(const char *env_h5_drvr) is_hdf5 = H5Fis_accessible(filename, fapl_id); VERIFY(is_hdf5, TRUE, "H5Fis_accessible"); + /*****************************************/ + /* Newly created file that is still open */ + /*****************************************/ + + /* On Windows, file locking is mandatory so this check ensures that + * H5Fis_accessible() works on files that have an exclusive lock. + * Previous versions of this API call created an additional file handle + * and attempted to read through it, which will not work when locks + * are enforced by the OS. + */ + + /* Create a file and hold it open */ + fid = H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, fapl_id); + CHECK(fid, H5I_INVALID_HID, "H5Fcreate"); + + /* Verify that the file is an HDF5 file */ + is_hdf5 = H5Fis_accessible(filename, fapl_id); + VERIFY(is_hdf5, TRUE, "H5Fis_accessible"); + + /* Close file */ + ret = H5Fclose(fid); + CHECK(ret, FAIL, "H5Fclose"); + /*******************************/ /* Non-default user block size */ /*******************************/ From cc1b66935a0a2addc639926298ab4097427d28f1 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 22 Mar 2021 00:03:30 +0000 Subject: [PATCH 6/7] Committing clang-format changes --- src/H5Fint.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/H5Fint.c b/src/H5Fint.c index 4a9c97f7bae..27d045aefad 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1056,10 +1056,10 @@ H5F_prefix_open_file(H5F_t *primary_file, H5F_prefix_open_t prefix_type, const c htri_t H5F__is_hdf5(const char *name, hid_t fapl_id) { - H5FD_t *file = NULL; /* Low-level file struct */ - H5F_shared_t *shared = NULL; /* Shared part of file */ - haddr_t sig_addr = HADDR_UNDEF; /* Addess of hdf5 file signature */ - htri_t ret_value = FAIL; /* Return value */ + H5FD_t * file = NULL; /* Low-level file struct */ + H5F_shared_t *shared = NULL; /* Shared part of file */ + haddr_t sig_addr = HADDR_UNDEF; /* Addess of hdf5 file signature */ + htri_t ret_value = FAIL; /* Return value */ FUNC_ENTER_PACKAGE From 2b827dfef3989d8a38808d2f7d90f2bff97c57ac Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Sun, 21 Mar 2021 17:08:30 -0700 Subject: [PATCH 7/7] Trivial change to force github to run actions --- src/H5Fint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/H5Fint.c b/src/H5Fint.c index 27d045aefad..6da9473eb25 100644 --- a/src/H5Fint.c +++ b/src/H5Fint.c @@ -1793,7 +1793,7 @@ H5F_open(const char *name, unsigned flags, hid_t fcpl_id, hid_t fapl_id) FUNC_ENTER_NOAPI(NULL) /* - * If the driver has a `cmp' method then the driver is capable of + * If the driver has a 'cmp' method then the driver is capable of * determining when two file handles refer to the same file and the * library can insure that when the application opens a file twice * that the two handles coordinate their operations appropriately.