libctf: clean up hashtab error handling mess

The dict and archive opening code in libctf is somewhat unusual, because
unlike everything else, it cannot report errors by setting an error on the
dict, because in case of error there isn't one.  They get passed an error
integer pointer that is set on error instead.

Inside ctf_bufopen this is implemented by calling ctf_set_open_errno and
passing it a positive error value.  In turn this means that most things it
calls (including init_static_types) return zero on success and a *positive*
ECTF_* or errno value on error.

This trickles down to ctf_dynhash_insert_type, which is used by
init_static_types to add newly-detected types to the name tables.  This was
returning the error value it received from a variety of functions without
alteration.  ctf_dynhash_insert conformed to this contract by returning a
positive value on error (usually OOM), which is unfortunate for multiple
reasons:

- ctf_dynset_insert returns a *negative* value
- ctf_dynhash_insert and ctf_dynset_insert don't take an fp, so the value
  they return is turned into the errno, so it had better be right, callers
  don't just check for != 0 here
- more or less every single caller of ctf_dyn*_insert in libctf other than
  ctf_dynhash_insert_type (and there are a *lot*, mostly in the
  deduplicator) assumes that ctf_dynhash_insert returns a negative value
  on error, even though it doesn't.  In practice the only possible error is
  OOM, but if OOM does happen we end up with a nonsense error value.

The simplest fix for this seems to be to make ctf_dynhash_insert and
ctf_dynset_insert conform to the usual interface contract: negative
values are errors.  This in turn means that ctf_dynhash_insert_type
needs to change: let's make it consistent too, returning a negative
value on error, putting the error on the fp in non-negated form.

init_static_types_internal adapts to this by negating the error return from
ctf_dynhash_insert_type, so the value handed back to ctf_bufopen is still
positive: the new call site in ctf_track_enumerator does not need to change.

(The existing tests for this reliably detect when I get it wrong.
I know, because they did.)

libctf/
	* ctf-hash.c (ctf_dynhash_insert): Negate return value.
	(ctf_dynhash_insert_type): Set de-negated error on the dict:
        return negated error.
	* ctf-open.c (init_static_types_internal): Adapt to this change.

diff --git a/libctf/ctf-hash.c b/libctf/ctf-hash.c
index 7f291e3ac27e1a1ebde9e1fd357f35d06cbaacf9..cd7473419cba12a8b6278c829b9e1ddfbc87e8d2 100644 (file)
--- a/libctf/ctf-hash.c
+++ b/libctf/ctf-hash.c
@@ -256,7 +256,7 @@ ctf_dynhash_insert (ctf_dynhash_t *hp, void *key, void *value)
                             key_free, value_free);
 
   if (!slot)
-    return errno;
+    return -errno;
 
   /* Keep track of the owner, so that the del function can get at the key_free
      and value_free functions.  Only do this if one of those functions is set:
@@ -786,7 +786,7 @@ ctf_dynhash_insert_type (ctf_dict_t *fp, ctf_dynhash_t *hp, uint32_t type,
     return EINVAL;
 
   if ((str = ctf_strptr_validate (fp, name)) == NULL)
-    return ctf_errno (fp);
+    return ctf_errno (fp) * -1;
 
   if (str[0] == '\0')
     return 0;             /* Just ignore empty strings on behalf of caller.  */
@@ -795,6 +795,9 @@ ctf_dynhash_insert_type (ctf_dict_t *fp, ctf_dynhash_t *hp, uint32_t type,
                                 (void *) (ptrdiff_t) type)) == 0)
     return 0;
 
+  /* ctf_dynhash_insert returns a negative error value: negate it for
+     ctf_set_errno.  */
+  ctf_set_errno (fp, err * -1);
   return err;
 }
 

diff --git a/libctf/ctf-open.c b/libctf/ctf-open.c
index 3f624987f9060dc441ca87897b6f5a91ff172e6f..942a53a8f2c17da3dc735889e366cb39ef187cbd 100644 (file)
--- a/libctf/ctf-open.c
+++ b/libctf/ctf-open.c
@@ -681,6 +681,8 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
    the latest supported representation in the process, if needed, and if this
    recension of libctf supports upgrading.
 
+   Returns zero on success and a *positive* ECTF_* or errno value on error.
+
    This is a wrapper to simplify memory allocation on error in the _internal
    function that does all the actual work.  */
 
@@ -886,7 +888,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                               LCTF_INDEX_TO_TYPE (fp, id, child),
                                               tp->ctt_name);
                if (err != 0)
-                 return err;
+                 return err * -1;
              }
            break;
          }
@@ -905,7 +907,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                         LCTF_INDEX_TO_TYPE (fp, id, child),
                                         tp->ctt_name);
          if (err != 0)
-           return err;
+           return err * -1;
          break;
 
        case CTF_K_STRUCT:
@@ -920,7 +922,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                         tp->ctt_name);
 
          if (err != 0)
-           return err;
+           return err * -1;
 
          break;
 
@@ -936,7 +938,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                         tp->ctt_name);
 
          if (err != 0)
-           return err;
+           return err * -1;
          break;
 
        case CTF_K_ENUM:
@@ -949,14 +951,14 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                           tp->ctt_name);
 
            if (err != 0)
-             return err;
+             return err * -1;
 
            /* Remember all enums for later rescanning.  */
 
            err = ctf_dynset_insert (all_enums, (void *) (ptrdiff_t)
                                     LCTF_INDEX_TO_TYPE (fp, id, child));
            if (err != 0)
-             return err;
+             return err * -1;
            break;
          }
 
@@ -968,7 +970,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                         LCTF_INDEX_TO_TYPE (fp, id, child),
                                         tp->ctt_name);
          if (err != 0)
-           return err;
+           return err * -1;
          break;
 
        case CTF_K_FORWARD:
@@ -985,7 +987,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                err = ctf_dynhash_insert_type (fp, h, LCTF_INDEX_TO_TYPE (fp, id, child),
                                               tp->ctt_name);
                if (err != 0)
-                 return err;
+                 return err * -1;
              }
            break;
          }
@@ -1010,7 +1012,7 @@ init_static_types_internal (ctf_dict_t *fp, ctf_header_t *cth,
                                         LCTF_INDEX_TO_TYPE (fp, id, child),
                                         tp->ctt_name);
          if (err != 0)
-           return err;
+           return err * -1;
          break;
        default:
          ctf_err_warn (fp, 0, ECTF_CORRUPT,