VOL refactor and cleanup #4856

qkoziol · 2024-09-18T22:40:50Z

Cleanup and prepare for thread-safety changes.

Big ideas:

Wrap H5VL_class_t with H5VL_connector_t, so use of the class can be refcounted within the H5VL package, instead of relying on storing an ID within the H5VL_t struct and incrementing & decrementing the ID's refcount.
Register H5VL_connector_t* for VOL connector IDs, instead of the H5VL_class_t*
Stop other packages from rummaging around inside H5VL_connector_t and H5VL_object_t data structures, so that the H5VL package can change implementation details without coupled changes throughout the library

Small things:

Simplified the coding for creating links
Moved some routines into more logical locations

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

So they don't sound like they are operating on connectors. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Also, small code tidying Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Also simplify and enforce some modularity boundaries around them, so that other packages can't modify VOL data structures. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

qkoziol · 2024-09-18T22:42:38Z

fortran/src/H5VLff.F90

+!!
+!! See C API: @ref H5VLcmp_connector_cls()
+!!
+  SUBROUTINE H5VLcmp_connector_cls_f(are_same, conn_id1, conn_id2, hdferr)


This routine is necessary because it's possible to have VOL connector IDs that are different, but point to the same underlying VOL connector class.

qkoziol · 2024-09-18T22:43:23Z

fortran/test/vol_connector.F90


       f_ptr = C_NULL_PTR
       CALL H5Pset_vol_f(fapl_id, vol_id, error, f_ptr)
       CALL check("H5Pset_vol_f",error,total_error)

       CALL H5Pget_vol_id_f(fapl_id, vol_id_out, error)
       CALL check("H5Pget_vol_id_f",error,total_error)
-       CALL VERIFY("H5Pget_vol_id_f", vol_id_out, vol_id, total_error)
+       are_same = .FALSE.


Use new routine instead of directly comparing IDs.

qkoziol · 2024-09-18T22:43:34Z

java/src/hdf/hdf5lib/H5.java

+    /**
+     * @ingroup JH5VL
+     *
+     * H5VLcmp_connector_cls Determines whether two connector identifiers refer to the same connector.


This routine is necessary because it's possible to have VOL connector IDs that are different, but point to the same underlying VOL connector class.

qkoziol · 2024-09-18T22:44:24Z

java/test/TestH5VL.java

@@ -98,7 +98,8 @@ public void testH5VLget_connector_id()
             */
            String connector = System.getenv("HDF5_VOL_CONNECTOR");
            if (connector == null)
-                assertEquals(HDF5Constants.H5VL_NATIVE, native_id);
+                assertTrue("H5.H5VLcmp_connector_cls(H5VL_NATIVE_NAME, native_id)",
+                           H5.H5VLcmp_connector_cls(HDF5Constants.H5VL_NATIVE, native_id));


Use new routine instead of directly comparing IDs.

qkoziol · 2024-09-18T22:46:32Z

src/H5A.c

@@ -125,7 +125,7 @@ H5A__create_common(H5VL_object_t *vol_obj, H5VL_loc_params_t *loc_params, const
        HGOTO_ERROR(H5E_ATTR, H5E_CANTINIT, H5I_INVALID_HID, "unable to create attribute");

    /* Register the new attribute and get an ID for it */
-    if ((ret_value = H5VL_register(H5I_ATTR, attr, vol_obj->connector, true)) < 0)
+    if ((ret_value = H5VL_register(H5I_ATTR, attr, H5VL_OBJ_CONNECTOR(vol_obj), true)) < 0)


These changes, and similar ones in non-H5VL packages, are to remove the code coupling to the H5VL package, allowing it to change implementation of H5VL data structures without rippling through the library.

qkoziol · 2024-09-18T22:49:07Z

src/H5CX.c

@@ -931,33 +931,27 @@ H5CX_retrieve_state(H5CX_state_t **api_state)
    } /* end if */

    /* Keep a copy of the VOL connector property, if there is one */
-    if ((*head)->ctx.vol_connector_prop_valid && (*head)->ctx.vol_connector_prop.connector_id > 0) {
+    if ((*head)->ctx.vol_connector_prop_valid && (*head)->ctx.vol_connector_prop.connector) {


VOL connector property stores a pointer to the connector struct now, instead of nesting an ID inside the property list.

qkoziol · 2024-09-18T22:51:54Z

src/H5F.c

@@ -88,12 +88,6 @@ static herr_t H5F__flush_api_common(hid_t object_id, H5F_scope_t scope, void **t
 /* Local Variables */
 /*******************/

-/* Declare a free list to manage the H5VL_t struct */


Unused (and should not be used outside the H5VL package)

src/H5VL.c

src/H5VLcallback.c

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

src/H5VLint.c

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

tools/lib/h5tools.c

jhendersonHDF · 2024-09-24T20:39:18Z

General comment: H5VLpeek_connector_id_by_name is used by the log-based VOL and H5VLpeek_connector_id_by_value is used by the REST and DAOS VOLs, so we'll want to decide how to handle that switchover gracefully.

release_docs/RELEASE.txt

qkoziol · 2024-09-25T17:48:19Z

General comment: H5VLpeek_connector_id_by_name is used by the log-based VOL and H5VLpeek_connector_id_by_value is used by the REST and DAOS VOLs, so we'll want to decide how to handle that switchover gracefully.

Yes. I added info to the RELEASE.txt note suggesting the correct action for developers who are using them. Anything else needed? We can update the connectors after the merge.

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

java/src/jni/h5Constants.c

Aligns them with the way the H5T, H5P, H5E packages return an ID back to an application, and removes calling an internal routine from the public header. Extra work for the internal passthru connector, to keep the main passthru source file using only public API calls. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

qkoziol · 2024-10-02T16:06:41Z

@jhendersonHDF - Any other comments for this PR? I'd like to get it merged ASAP after the 1.16 branch is created, so I can queue the next large one up.

jhendersonHDF · 2024-10-02T17:23:44Z

@qkoziol I don't think I have any other complaints in particular, though I believe @fortnern mentioned he still needed to review it and I also believe we decided to discuss this PR at the next working group meeting.

qkoziol · 2024-10-02T21:59:02Z

I've already discussed it at last week's meeting, although I'm happy to run through it again. I believe it's ready for any final review and merge.

qkoziol and others added 12 commits September 6, 2024 17:47

Rename H5VL_t to H5VL_connector_t

413c620

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Switch parameter to H5VL__register_connector

efe6746

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Change name of routines that operate on connector property objects

6cfc479

So they don't sound like they are operating on connectors. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Refactor connector object create & free into routines

5c4641f

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Use new connector free routine for freeing new connector object

360a585

Also, small code tidying Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Track list of active connectors with global linked list

4686d86

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Switch to searching list of active VOL connectors, instead of IDs

214fd85

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Merge remote-tracking branch 'origin/develop' into vol_cleanup

97a5861

Refactor VOL data structures to remove internal use of IDs

fe4a620

Also simplify and enforce some modularity boundaries around them, so that other packages can't modify VOL data structures. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Committing clang-format changes

c3b9656

Spelling

e3e48f0

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Fix link errors in Java wrapper

a53ee18

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

qkoziol requested review from jhendersonHDF, byrnHDF, derobins, brtnfld, epourmal, lrknox, fortnern, vchoi-hdfgroup, bmribler, glennsong09 and mattjala as code owners September 18, 2024 22:40

qkoziol commented Sep 18, 2024

View reviewed changes

mattjala reviewed Sep 19, 2024

View reviewed changes

src/H5VL.c Show resolved Hide resolved

mattjala reviewed Sep 19, 2024

View reviewed changes

src/H5VLcallback.c Show resolved Hide resolved

Address review comments

63bb68b

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

mattjala reviewed Sep 19, 2024

View reviewed changes

src/H5VLint.c Outdated Show resolved Hide resolved

Fix comment

4e2bbbb

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

mattjala reviewed Sep 19, 2024

View reviewed changes

tools/lib/h5tools.c Show resolved Hide resolved

Merge remote-tracking branch 'origin/develop' into vol_cleanup

cfb1d87

mattjala approved these changes Sep 19, 2024

View reviewed changes

qkoziol added 2 commits September 23, 2024 17:04

Merge remote-tracking branch 'origin/develop' into vol_cleanup

1ba58be

Merge remote-tracking branch 'origin/develop' into vol_cleanup

aeeec14

jhendersonHDF reviewed Sep 24, 2024

View reviewed changes

release_docs/RELEASE.txt Outdated Show resolved Hide resolved

qkoziol added 2 commits September 25, 2024 12:50

Remove merge trash

c49d15c

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Merge remote-tracking branch 'origin/develop' into vol_cleanup

4033e6c

jhendersonHDF reviewed Sep 26, 2024

View reviewed changes

java/src/jni/h5Constants.c Outdated Show resolved Hide resolved

qkoziol and others added 7 commits September 26, 2024 15:41

Merge remote-tracking branch 'origin/develop' into vol_cleanup

10839a4

Committing clang-format changes

1be1917

Merge remote-tracking branch 'origin/develop' into vol_cleanup

516e020

Fixe speeling eror

439024d

Signed-off-by: Quincey Koziol <quincey@koziol.cc>

Merge remote-tracking branch 'origin/develop' into vol_cleanup

07355ec

Merge remote-tracking branch 'origin/develop' into vol_cleanup

6406498

Merge remote-tracking branch 'origin/develop' into vol_cleanup

8e0058c

fortnern approved these changes Oct 3, 2024

View reviewed changes

derobins merged commit 767282f into HDFGroup:develop Oct 3, 2024
60 checks passed

qkoziol deleted the vol_cleanup branch October 3, 2024 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VOL refactor and cleanup #4856

VOL refactor and cleanup #4856

qkoziol commented Sep 18, 2024 •

edited

Loading

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

qkoziol Sep 18, 2024

jhendersonHDF commented Sep 24, 2024

qkoziol commented Sep 25, 2024

qkoziol commented Oct 2, 2024

jhendersonHDF commented Oct 2, 2024

qkoziol commented Oct 2, 2024

VOL refactor and cleanup #4856

VOL refactor and cleanup #4856

Conversation

qkoziol commented Sep 18, 2024 • edited Loading

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

qkoziol Sep 18, 2024

Choose a reason for hiding this comment

jhendersonHDF commented Sep 24, 2024

qkoziol commented Sep 25, 2024

qkoziol commented Oct 2, 2024

jhendersonHDF commented Oct 2, 2024

qkoziol commented Oct 2, 2024

qkoziol commented Sep 18, 2024 •

edited

Loading