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)

qkoziol · 2024-09-18T23:38:15Z

src/H5VLnative.h

@@ -25,9 +25,6 @@
 /* Public Macros */
 /*****************/

-/* Identifier for the native VOL connector */


Move private pieces to private header

qkoziol · 2024-09-18T23:39:25Z

src/H5VLpkg.h

@@ -36,37 +36,51 @@
 /* Package Private Typedefs */
 /****************************/

+/* Internal struct to track VOL connectors */
+struct H5VL_connector_t {


One connector object per VOL connector class registered with the library

qkoziol · 2024-09-18T23:40:13Z

src/H5VLprivate.h

@@ -27,28 +27,33 @@
 /* Library Private Macros */
 /**************************/

+/* If the module using this macro is allowed access to the private variables, access them directly */
+#ifdef H5VL_MODULE
+#define H5VL_OBJ_RC(VOL_OBJ)         ((VOL_OBJ)->rc)


Getters, for the rest of the library packages. (Similar idiom as in H5Fprivate.h)

qkoziol · 2024-09-18T23:41:15Z

test/event_set.c

@@ -395,7 +396,12 @@ test_es_get_requests(void)
        TEST_ERROR;
    if (count != 1)
        TEST_ERROR;
-    if (connector_ids[0] != connector_ids_g[0])
+    cmp_value = 0;
+    if (H5VLcmp_connector_cls(&cmp_value, connector_ids[0], connector_ids_g[0]) < 0)


Use new comparison routine in tests.

qkoziol · 2024-09-18T23:46:00Z

@mattjala - this is the PR that we talked about a week and a half ago. Should replace #4635 and fix #4634

src/H5ES.c

src/H5Fprivate.h

src/H5VLconnector.h

src/H5L.c

mattjala · 2024-09-19T15:41:36Z

src/H5L.c

        HGOTO_ERROR(H5E_LINK, H5E_CANTCREATE, FAIL, "unable to create hard link");

+    /* Set the connector to use for async operations */
+    if (connector)
+        *connector = (link_vol_obj ? H5VL_OBJ_CONNECTOR(link_vol_obj) : H5VL_OBJ_CONNECTOR(curr_vol_obj));


Since this returns a pointer to the connector, I think it should increment the ref count and require the caller to release it after it's done.

It's strictly local within the source module and only used in one spot, so I don't think that is necessary here.

src/H5Pfapl.c

src/H5VL.c

mattjala · 2024-09-19T17:42:49Z

src/H5VLcallback.c

        HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a VOL connector ID");

-    /* Bypass the H5VLint layer, calling the VOL callback directly */


What was the original reason to bypass the H5VLint layer here?

Not certain 🤷‍♂️

Better to have it be consistent, in the long term

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

src/H5VLint.c

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

mattjala · 2024-09-19T19:16:13Z

tools/lib/h5tools.c

@@ -671,12 +671,8 @@ h5tools_set_fapl_vol(hid_t fapl_id, h5tools_vol_info_t *vol_info)
                /* Check for VOL connectors that ship with the library, then try
                 * registering by name if that fails.
                 */
-                if (!strcmp(vol_info->u.name, H5VL_NATIVE_NAME)) {


This looks like it would cause problems when trying to use h5tools with the native VOL

Tools use the native connector by default; no command-line option is necessary.

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

byrnHDF approved these changes Sep 19, 2024

View reviewed changes