Refactor error handling code to eliminate internal ID calls #4453

qkoziol · 2024-05-01T22:39:30Z

All calls to the H5I routines are now made in API routines (sometimes in FUNC_ENTER/LEAVE_* macros), except for some calls to H5E_clear_stack() within the library, but I'm planning to remove those over time.

Also, made all the library internal error messages into static const variables, instead of malloc'ing them, which means that they can just be referenced and not copied.

Several new and updated auto-generated header files were necessary to enable this.

All calls to the H5I routines are now made in API routines (sometimes in FUNC_ENTER/LEAVE_* macros), except for some calls to H5E_clear_stack() within the library, but I'm planning to remove those over time. Also, made all the library internal error messages into static const variables, instead of malloc'ing them, which means that they can just be referenced and not copied. Several new and updated auto-generated header files were necessary to enable this. Signed-off-by: Quincey Koziol <quincey@koziol.cc>

derobins · 2024-05-02T12:13:09Z

If we're going to check in the generated header files, we should do that for the other files as well (in a separate PR) and create a GitHub action to complain if they need to be regenerated (in another PR).

qkoziol · 2024-05-02T12:42:50Z

Ah, I thought the generated H5E headers were included in the repo. I'll take them out.

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

qkoziol · 2024-05-02T12:45:32Z

Ah, I thought the generated H5E headers were included in the repo. I'll take them out.

Removed

qkoziol · 2024-05-02T12:49:38Z

bin/make_err

        printf HEADER "H5_DLLVAR hid_t %-20s /* %s */\n","${name}_g;",$major{$name};
    }

    # Iterate over all the minor error sections
    print HEADER "\n/*********************/\n";
    print HEADER   "/* Minor error codes */\n";
    print HEADER   "/*********************/\n";
-    while ( ($sect_name, $sect_desc) = each (%section)) {
-        print HEADER "\n/* $sect_desc */\n";
+    foreach $sect_name (sort keys %section) {


Sort the errors alphabetically by name, so they have stable lists in the generated headers.

qkoziol · 2024-05-02T12:50:35Z

bin/make_err

+##############################################################################
+# Create the generated portion of the H5E major message definition code
+#
+sub create_majdef ($) {


This creates a static const message for each library-internal major error message, so they aren't malloc'ed/free'd at init/term.

qkoziol · 2024-05-02T12:50:47Z

bin/make_err

+##############################################################################
+# Create the generated portion of the H5E minor message definition code
+#
+sub create_mindef ($) {


This creates a static const message for each library-internal minor error message, so they aren't malloc'ed/free'd at init/term.

qkoziol · 2024-05-02T12:51:32Z

bin/make_err

+    foreach $name (sort keys %major) {
+        print HEADER "/* $name */\n";
+        print HEADER " "x(0*$indent),"assert(H5I_INVALID_HID == ${name}_g);\n";
+        print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, &${name}_msg_s, false)) < 0)\n";


Set up the major error IDs, to use the static variables.

qkoziol · 2024-05-02T12:51:45Z

bin/make_err

+        for $name (sort @{$section_list{$sect_name}}) {
+            print HEADER "/* $name */\n";
+            print HEADER " "x(0*$indent),"assert(H5I_INVALID_HID == ${name}_g);\n";
+            print HEADER " "x(0*$indent),"if((${name}_g = H5I_register(H5I_ERROR_MSG, &${name}_msg_s, false)) < 0)\n";


Set up the minor error IDs, to use the static variables.

qkoziol · 2024-05-02T12:52:50Z

src/H5D.c

@@ -2137,7 +2137,7 @@ H5Dformat_convert(hid_t dset_id)

    /* Convert the dataset */
    if (H5VL_dataset_optional(vol_obj, &vol_cb_args, H5P_DATASET_XFER_DEFAULT, H5_REQUEST_NULL) < 0)
-        HGOTO_ERROR(H5E_DATASET, H5E_INTERNAL, FAIL, "can't convert dataset format");
+        HGOTO_ERROR(H5E_DATASET, H5E_CANTUPDATE, FAIL, "can't convert dataset format");


Use minor error code instead of major.

qkoziol · 2024-05-02T12:53:02Z

src/H5D.c

@@ -2442,7 +2442,7 @@ H5Dchunk_iter(hid_t dset_id, hid_t dxpl_id, H5D_chunk_iter_op_t op, void *op_dat

    /* Iterate over the chunks */
    if ((ret_value = H5VL_dataset_optional(vol_obj, &vol_cb_args, dxpl_id, H5_REQUEST_NULL)) < 0)
-        HERROR(H5E_BADITER, H5E_BADITER, "error iterating over dataset chunks");
+        HERROR(H5E_DATASET, H5E_BADITER, "error iterating over dataset chunks");


Use major error code instead of minor.

qkoziol · 2024-05-02T12:53:49Z

src/H5Dint.c

@@ -1495,7 +1495,7 @@ H5D_open(const H5G_loc_t *loc, hid_t dapl_id)
    /* Check if dataset was already open */
    if (NULL == (shared_fo = (H5D_shared_t *)H5FO_opened(dataset->oloc.file, dataset->oloc.addr))) {
        /* Clear any errors from H5FO_opened() */
-        H5E_clear_stack(NULL);
+        H5E_clear_stack();


Internal to the library, we always clear the default error stack, so update H5E_clear_stack() to do that.

qkoziol · 2024-05-02T12:55:21Z

src/H5Eprivate.h

 /*
 * HERROR macro, used to facilitate error reporting between a FUNC_ENTER()
 * and a FUNC_LEAVE() within a function body.  The arguments are the major
 * error number, the minor error number, and a description of the error.
 */
 #define HERROR(maj_id, min_id, ...)                                                                          \
    do {                                                                                                     \
-        H5E_printf_stack(NULL, __FILE__, __func__, __LINE__, H5E_ERR_CLS_g, maj_id, min_id, __VA_ARGS__);    \
+        H5E_printf_stack(__FILE__, __func__, __LINE__, maj_id, min_id, __VA_ARGS__);                         \


Internal errors always use the default error stack and the library's error class.

qkoziol · 2024-05-02T13:02:16Z

src/H5public.h

@@ -88,6 +88,10 @@
 * For pre-releases like \c snap0. Empty string for official releases.
 */
 #define H5_VERS_SUBRELEASE ""
+/**


Concise version string, for library's error class.

qkoziol · 2024-05-02T13:03:14Z

src/H5E.c

Moved all the internal routines to the H5Eint.c file, to reflect current library coding style.

qkoziol · 2024-05-02T13:04:49Z

src/H5E.c

-    if (HDvasprintf(&tmp, fmt, ap) < 0)
-        HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed");
+    /* Duplicate string information */
+    if (NULL == (tmp_file = strdup(file)))


Duplicate strings and increment IDs for application error push operations. Do this here, in the API routine, instead of within the library.

qkoziol · 2024-05-02T13:05:19Z

src/H5Edeprec.c


    /* Don't clear the error stack! :-) */
    FUNC_ENTER_API_NOCLEAR(FAIL)

+    /* Duplicate string information */


Same as H5Epush2(): Duplicate strings and increment IDs for application error push operations. Do this here, in the API routine, instead of within the library.

qkoziol · 2024-05-02T13:06:43Z

src/H5Eint.c

Nearly all of the routines moved here are unmodified. I've added comments to the moved ones that changed.

qkoziol · 2024-05-02T13:07:30Z

src/H5Eint.c

+
+/* Library's error class */
+static const H5E_cls_t H5E_err_cls_s = {false, H5E_CLS_NAME, H5E_CLS_LIB_NAME, H5_VERS_STR};
+


Autogenerated headers that define the major and minor error messages for the library. Used to register the library's error code IDs.

qkoziol · 2024-05-02T13:08:42Z

src/H5Eint.c

+    assert(err);
+
+    /* Free resources, if application registered this message */
+    if (err->app_msg)


Only application error messages need to be freed, the library error messages are statically declared variables.

qkoziol · 2024-05-02T13:09:42Z

src/H5Eint.c

+    /* Copy new stack to current error stack */
+    current_stack->nused = estack->nused;
+    for (u = 0; u < current_stack->nused; u++)
+        if (H5E__copy_stack_entry(&current_stack->entries[u], &estack->entries[u]) < 0)


Refactored all the routines that manipulate stack entries to use a new, common stack entry copy routine.

qkoziol · 2024-05-02T13:11:28Z

src/H5Eint.c

@@ -658,22 +1410,13 @@ H5E_printf_stack(H5E_t *estack, const char *file, const char *func, unsigned lin
    va_start(ap, fmt);
    va_started = true;

-    /* Use the vasprintf() routine, since it does what we're trying to do below */
-    if (HDvasprintf(&tmp, fmt, ap) < 0)


Rather than call vasprintf() here, then have H5E__push_stack() duplicate the string, just have H5E__push_stack() (H5E__set_stack_entry(), actually) call vasprintf().

qkoziol · 2024-05-02T13:14:36Z

src/H5Eint.c

+    FUNC_LEAVE_NOAPI(ret_value)
+} /* end H5E_clear_stack() */
+
+/*-------------------------------------------------------------------------


This is analogous to H5E_clear_stack(), but is internal to the H5E package, to allow clearing any error stack.

qkoziol · 2024-05-06T15:08:10Z

Can I get some reviews on this PR? I'd like to get this change to the H5E package in, so I can work on refactoring the H5E_clear_stack() calls out of the internal library code with this refactor underneath those updates.

src/H5Eint.c

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

bin/make_err

.gitignore

jhendersonHDF

My perl isn't exactly strong, but everything looks reasonable to me

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

qkoziol · 2024-05-08T17:21:14Z

My perl isn't exactly strong, but everything looks reasonable to me

Super, thanks!

…#4453) All calls to the H5I routines are now made in API routines (sometimes in FUNC_ENTER/LEAVE_* macros), except for some calls to H5E_clear_stack() within the library, but I'm planning to remove those over time. Also, made all the library internal error messages into static const variables, instead of malloc'ing them, which means that they can just be referenced and not copied. Several new and updated auto-generated header files were necessary to enable this.

* win32defs: Fix Wundef warning (#4467) * Refactor error handling code to eliminate internal ID calls (#4453) All calls to the H5I routines are now made in API routines (sometimes in FUNC_ENTER/LEAVE_* macros), except for some calls to H5E_clear_stack() within the library, but I'm planning to remove those over time. Also, made all the library internal error messages into static const variables, instead of malloc'ing them, which means that they can just be referenced and not copied. Several new and updated auto-generated header files were necessary to enable this. * CMake: Fix mingw/fortran build (#4466) * Update for blosc2 in plugins and prefix hdf5 cmake varnames (#4468) * Fix an issue where compound datatype member IDs can be leaked during conversion (#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown * H5Group: Fix operator= (#4473) Closes #4472 * Fix github issue #2523: doxygen -- fix grammatically incorrect sentence alias (#4474) * Remove env step not used by CI in testing (#4476) * Add H5fortkit dependecy for H5Rff.F90 (#4482) * Properly clean up cache when failing to load an object header (#4477) * Properly clean up cache when failing to load an object header * Don't check message type a second time in H5G__open_oid if the first attempt returns error * Add more asserts to H5O__assert() to avoid segfaults --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Add a missing image from the original document (#4490) * Disable EOF checks for SWMR readers in more cases. (#4489) Fixes a race condition where the reader opens the file and sets its EOF from the file's size (from the stat() call in the driver open callback). Then, before the reader can read the file's superblock, a SWMR writer races in, extends the file, and closes the file, writing an updated superblock with the 'writer' and 'SWMR writer' flags in the superblock off (appropriately). Then the reader proceeds to read the superblock, and flags the EOF as wrong. Taking out the check for the 'writer' and 'SWMR writer' flags will cause SWMR readers to avoid flagging the file as incorrect. * Remove unnecessary fortran install (#4498) * Only one version of binaries is produced for platforms (#4496) * Fix for github issue #2220. (#4497) Document the limitation in the Passthrough Conncector section of the VOL Connector Author Guide. The limitation is posted by Neil in the github issue on Dec 22, 2022. * Release asset tarballs with no version filenames (#4494) * Improve spec. reading superblock into cache (a little) by using v2 size (#4491) * Improve spec. reading superblock into cache (a little) by using v2 size Instead of reading the absolute minimal possible, use the likely value of a v2+ superblock w/8-byte addresses & lengths. * Fix for github Issue #1388 can't delete renamed dense attribute with corder tracking enabled (#4462) * Fix for github issue #1388: can't delete renamed dense attribute with corder tracking enabled The problem occurs in step 3(b) below which will delete the attribute with corder x from the creation order index v2 B-tree. The rename sequence in H5A__dense_rename() occurs in the following order: 1) The old attribute with corder x was removed from the creation order index v2 B-tree 2) The new renamed attribute was inserted via H5A__dense_insert(): (a) insert the attribute with new name j into the name index v2 B-tree (b) insert the attribute with corder x into the creation order index v2 B-tree 3) The old attribute was removed via H5A__dense_remove(): (a) remove the attribute with old name k from the name index v2 B-tree (b) remove the attribute with coorder x from the creation order index v2 B-tree Fix: deactivate the "corder_bt2_addr" field so that H5A__dense_remove() won't delete the attribute with corder x from the creation order index v2 B-tree. * Fix/revert a libtool sed hack (#4501) * Revert "Remove Autotools sed hack (#3848)" This reverts commit 8b3ffde. * Fix libtool sed cleanup on MacOS Convert sed -i line to sed > libtool.bak && mv libtool.bak libtool to avoid non-portable -i option. * Update src/H5public.h * Set H5 specific vars immediately if legacy find (#4512) * Correct find process vars (vs in-line build) * Correct SZIP find * Everything is libaec 1.0.6 or newer * Correct option help text

qkoziol and others added 2 commits May 1, 2024 16:52

Committing clang-format changes

06118ad

qkoziol requested review from lrknox, derobins, byrnHDF, fortnern, jhendersonHDF, vchoi-hdfgroup, bmribler, glennsong09, mattjala and brtnfld as code owners May 1, 2024 22:39

Remove generated H5E headers

a5dcea4

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

qkoziol commented May 2, 2024

View reviewed changes

src/H5E.c

Copy link

Contributor Author

qkoziol May 2, 2024

Choose a reason for hiding this comment

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

Moved all the internal routines to the H5Eint.c file, to reflect current library coding style.

qkoziol commented May 2, 2024

View reviewed changes

src/H5Eint.c

Copy link

Contributor Author

qkoziol May 2, 2024

Choose a reason for hiding this comment

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

Nearly all of the routines moved here are unmodified. I've added comments to the moved ones that changed.

qkoziol commented May 2, 2024

View reviewed changes

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

4eaadb7

jhendersonHDF reviewed May 7, 2024

View reviewed changes

src/H5Eint.c Outdated Show resolved Hide resolved

qkoziol and others added 4 commits May 7, 2024 16:53

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

6057db9

Add new generated error headers to .gitignore

ebd8051

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

Update H5E__copy_stack_entry() to reflect its actual requirements

2d380a3

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

Committing clang-format changes

a0bd026

jhendersonHDF reviewed May 8, 2024

View reviewed changes

bin/make_err Outdated Show resolved Hide resolved

jhendersonHDF reviewed May 8, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

jhendersonHDF approved these changes May 8, 2024

View reviewed changes

qkoziol added 2 commits May 8, 2024 12:18

Correct typo

1521c90

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

Remove unused variable

752523a

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

derobins approved these changes May 9, 2024

View reviewed changes

derobins merged commit 2b5769e into HDFGroup:develop May 9, 2024
58 checks passed

qkoziol deleted the h5e_refactor branch May 9, 2024 14:50

jhendersonHDF mentioned this pull request May 22, 2024

Sync develop changes May 6 - May 20 to hdf5_1_14 #4503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor error handling code to eliminate internal ID calls #4453

Refactor error handling code to eliminate internal ID calls #4453

qkoziol commented May 1, 2024

derobins commented May 2, 2024

qkoziol commented May 2, 2024

qkoziol commented May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024

qkoziol May 2, 2024 •

edited

Loading

qkoziol May 2, 2024

qkoziol commented May 6, 2024

jhendersonHDF left a comment

qkoziol commented May 8, 2024


		/* Library's error class */
		static const H5E_cls_t H5E_err_cls_s = {false, H5E_CLS_NAME, H5E_CLS_LIB_NAME, H5_VERS_STR};

Refactor error handling code to eliminate internal ID calls #4453

Refactor error handling code to eliminate internal ID calls #4453

Conversation

qkoziol commented May 1, 2024

derobins commented May 2, 2024

qkoziol commented May 2, 2024

qkoziol commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qkoziol May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qkoziol commented May 6, 2024

jhendersonHDF left a comment

Choose a reason for hiding this comment

qkoziol commented May 8, 2024

qkoziol May 2, 2024 •

edited

Loading