Always verify fr_sbuff_term_t order when using WITH_VERIFY_PTR

and remove calls to qsort.  Since the input arrays must be sorted,
we can simple do an O(n+m) walk over the input arrays

diff --git a/src/lib/util/sbuff.c b/src/lib/util/sbuff.c
index f36658bdda6b302a9abf977ff28396a54322a42e..b590e567ab380154124d827e3d2a8a6d9bf0e856 100644 (file)
--- a/src/lib/util/sbuff.c
+++ b/src/lib/util/sbuff.c
@@ -560,15 +560,12 @@ static inline bool fr_sbuff_terminal_search(fr_sbuff_t *in, char const *p,
 
 /** Compare two terminal elements for ordering purposes
  *
- * @param[in] one              first terminal to compare.
- * @param[in] two      second terminal to compare.
- * @return CMP(one, two)
+ * @param[in] a        first terminal to compare.
+ * @param[in] b                second terminal to compare.
+ * @return CMP(a,b)
  */
-static inline int8_t terminal_cmp(void const *one, void const *two)
+static inline int8_t terminal_cmp(fr_sbuff_term_elem_t const *a, fr_sbuff_term_elem_t const *b)
 {
-       fr_sbuff_term_elem_t const      *a = one;
-       fr_sbuff_term_elem_t const      *b = two;
-
        MEMCMP_RETURN(a, b, str, len);
        return 0;
 }
@@ -593,51 +590,48 @@ static void fr_sbuff_terminal_debug_tmp(fr_sbuff_term_elem_t const *elem[], size
  */
 fr_sbuff_term_t *fr_sbuff_terminals_amerge(TALLOC_CTX *ctx, fr_sbuff_term_t const *a, fr_sbuff_term_t const *b)
 {
-       size_t                          i, num, used, check;
+       size_t                          i, j, num;
        fr_sbuff_term_t                 *out;
        fr_sbuff_term_elem_t const      *tmp[UINT8_MAX + 1];
 
-       num = 0;
-       for (i = 0; i < a->len; i++) tmp[num++] = &a->elem[i];
-       for (i = 0; i < b->len; i++) tmp[num++] = &b->elem[i];
-
        /*
         *      Check all inputs are pre-sorted.  It doesn't break this
         *      function, but it's useful in case the terminal arrays
-        *      are used elsewhere without merging.
+        *      are defined elsewhere without merging.
         */
-#if !defined(NDEBUG) && defined(SBUFF_CHECK_TERM_ORDER)
-       if (a->len > 1) {
-               for (i = 0; i < a->len - 1; i++) {
-                       fr_assert(terminal_cmp(&a->elem[i], &a->elem[i + 1]) < 0);
-               }
-       }
-
-       if (b->len > 1) {
-               for (i = 0; i < b->len - 1; i++) {
-                       fr_assert(terminal_cmp(&b->elem[i], &b->elem[i + 1]) < 0);
-               }
-       }
+#if !defined(NDEBUG) && defined(WITH_VERIFY_PTR)
+       for (i = 0; i < a->len - 1; i++) fr_assert(terminal_cmp(&a->elem[i], &a->elem[i + 1]) < 0);
+       for (i = 0; i < b->len - 1; i++) fr_assert(terminal_cmp(&b->elem[i], &b->elem[i + 1]) < 0);
 #endif
 
-       used = num;
-       if (likely(num > 1)) {
-               fr_quick_sort((void const **)tmp, 0, num - 1, terminal_cmp);
-
-               for (i = 0; i < (num - 1); i++) {
-                       fr_assert(tmp[i] != NULL);
+       /*
+        *      Since the inputs are sorted, we can just do an O(n+m)
+        *      walk through the arrays, comparing entries across the
+        *      two arrays.
+        *
+        *      If there are duplicates, we prefer "a", for no particular reason.
+        */
+       num = i = j = 0;
+       while ((i < a->len) && (j < b->len)) {
+               int8_t cmp;
 
-                       /*
-                        *      Duplicates are adjacent, and can be omitted.
-                        */
-                       if (terminal_cmp(tmp[i], tmp[i + 1]) == 0) {
-                               used--;
-                               tmp[i] = NULL;
-                       }
+               cmp = terminal_cmp(&a->elem[i], &b->elem[j]);
+               if (cmp <= 0) {
+                       tmp[num++] = &a->elem[i++];
+               } else {
+                       tmp[num++] = &b->elem[j++];
                }
+
+               fr_assert(num <= UINT8_MAX);
        }
 
-       out = talloc_pooled_object(ctx, fr_sbuff_term_t, used, used * sizeof(fr_sbuff_term_elem_t));
+       /*
+        *      Only one of these will be hit, and it's simpler than nested "if" statements.
+        */
+       while (i < a->len) tmp[num++] = &a->elem[i++];
+       while (j < b->len) tmp[num++] = &b->elem[j++];
+
+       out = talloc_pooled_object(ctx, fr_sbuff_term_t, num, num * sizeof(fr_sbuff_term_elem_t));
        if (unlikely(!out)) return NULL;
 
        out->elem = talloc_array(out, fr_sbuff_term_elem_t, num);
@@ -645,15 +639,13 @@ fr_sbuff_term_t *fr_sbuff_terminals_amerge(TALLOC_CTX *ctx, fr_sbuff_term_t cons
                talloc_free(out);
                return NULL;
        }
-       out->len = used;
+       out->len = num;
 
-       check = 0;      /* How many non-nulls we've found */
-       for (i = 0; i < num; i++) {
-               if (!tmp[i]) continue;
+       for (i = 0; i < num; i++) out->elem[i] = *tmp[i]; /* copy merged results back */
 
-               out->elem[check++] = *tmp[i];
-       }
-       fr_assert(check == used);
+#if !defined(NDEBUG) && defined(WITH_VERIFY_PTR)
+       for (i = 0; i < num - 1; i++) fr_assert(terminal_cmp(&out->elem[i], &out->elem[i + 1]) < 0);
+#endif
 
        return out;
 }

diff --git a/src/lib/util/sbuff.h b/src/lib/util/sbuff.h
index b9259b503a70deb114895abad364522598c9af8e..efa235e853f10965e2b17f0bd7eb72fc4004763a 100644 (file)
--- a/src/lib/util/sbuff.h
+++ b/src/lib/util/sbuff.h
@@ -138,6 +138,8 @@ typedef struct {
 
 /** Set of terminal elements
  *
+ *  The elements MUST be listed in sorted order.  If the inputs are
+ *  not sorted, then all kinds of things will break.
  */
 typedef struct {
        size_t                  len;                    //!< Length of the list.