Fix DHCP attribute ordering

diff --git a/src/include/pair.h b/src/include/pair.h
index d7598441e25ef8d9bda0f742b8b0b2cf953266e5..70f68507d27b7419a51f24724b5787116585c49f 100644 (file)
--- a/src/include/pair.h
+++ b/src/include/pair.h
@@ -257,6 +257,7 @@ typedef             int8_t (*fr_cmp_t)(void const *a, void const *b);
  */
 #define                fr_pair_cmp_op(_op, _a, _b)     value_box_cmp_op(_op, _a->da->type, &_a->data, _b->da->type, &_b->data)
 int8_t         fr_pair_cmp_by_da_tag(void const *a, void const *b);
+int8_t         fr_pair_cmp_by_parent_num_tag(void const *a, void const *b);
 int            fr_pair_cmp(VALUE_PAIR *a, VALUE_PAIR *b);
 int            fr_pair_list_cmp(VALUE_PAIR *a, VALUE_PAIR *b);
 void           fr_pair_list_sort(VALUE_PAIR **vps, fr_cmp_t cmp);

diff --git a/src/lib/pair.c b/src/lib/pair.c
index 26759fe4eea420c6fb2059f51fd5cbdf3ad8a5e6..56e292683ab045aeafce6a6e1a84a47705f03059 100644 (file)
--- a/src/lib/pair.c
+++ b/src/lib/pair.c
@@ -903,16 +903,28 @@ void fr_pair_delete_by_num(VALUE_PAIR **head, unsigned int vendor, unsigned int
        }
 }
 
+/** Order attributes by their da, and tag
+ *
+ * Useful where attributes need to be aggregated, but not necessarily
+ * ordered by attribute number.
+ *
+ * @param[in] a                first dict_attr_t.
+ * @param[in] b                second dict_attr_t.
+ * @return
+ *     - +1 if a > b
+ *     - 0 if a == b
+ *     - -1 if a < b
+ */
 int8_t fr_pair_cmp_by_da_tag(void const *a, void const *b)
 {
        VALUE_PAIR const *my_a = a;
        VALUE_PAIR const *my_b = b;
 
+       uint8_t cmp;
+
        VERIFY_VP(my_a);
        VERIFY_VP(my_b);
 
-       uint8_t cmp;
-
        cmp = fr_pointer_cmp(my_a->da, my_b->da);
        if (cmp != 0) return cmp;
 
@@ -923,6 +935,81 @@ int8_t fr_pair_cmp_by_da_tag(void const *a, void const *b)
        return 0;
 }
 
+/** Order attributes by their attribute number, and tag
+ *
+ * @param[in] a                first dict_attr_t.
+ * @param[in] b                second dict_attr_t.
+ * @return
+ *     - +1 if a > b
+ *     - 0 if a == b
+ *     - -1 if a < b
+ */
+static inline int8_t pair_cmp_by_num_tag(void const *a, void const *b)
+{
+       VALUE_PAIR const *my_a = a;
+       VALUE_PAIR const *my_b = b;
+
+       VERIFY_VP(my_a);
+       VERIFY_VP(my_b);
+
+       if (my_a->da->attr < my_b->da->attr) return -1;
+       if (my_a->da->attr > my_b->da->attr) return +1;
+
+       if (my_a->tag < my_b->tag) return -1;
+       if (my_a->tag > my_b->tag) return +1;
+
+       return 0;
+}
+
+/** Order attributes by their parent(s), attribute number, and tag
+ *
+ * Useful for some protocols where attributes of the same number should by aggregated
+ * within a packet or container TLV.
+ *
+ * @param[in] a                first dict_attr_t.
+ * @param[in] b                second dict_attr_t.
+ * @return
+ *     - +1 if a > b
+ *     - 0 if a == b
+ *     - -1 if a < b
+ */
+int8_t fr_pair_cmp_by_parent_num_tag(void const *a, void const *b)
+{
+       VALUE_PAIR const        *vp_a = a;
+       VALUE_PAIR const        *vp_b = b;
+       fr_dict_attr_t const    *da_a = vp_a->da;
+       fr_dict_attr_t const    *da_b = vp_b->da;
+       fr_dict_attr_t const    *tlv_stack_a[FR_DICT_MAX_TLV_STACK + 1];
+       fr_dict_attr_t const    *tlv_stack_b[FR_DICT_MAX_TLV_STACK + 1];
+
+       /*
+        *      Fast path (assuming attributes
+        *      are in the same dictionary).
+        */
+       if ((da_b->parent->flags.is_root) && (da_b->parent->flags.is_root)) return pair_cmp_by_num_tag(vp_a, vp_b);
+
+       fr_proto_tlv_stack_build(tlv_stack_a, da_a);
+       fr_proto_tlv_stack_build(tlv_stack_b, da_b);
+
+       for (da_a = tlv_stack_a[0], da_b = tlv_stack_b[0]; da_a && da_b; da_a++, da_b++) {
+               if (da_a->attr > da_b->attr) return +1;
+               if (da_b->attr < da_b->attr) return -1;
+       }
+
+       /*
+        *      If a has a shallower attribute
+        *      hierarchy than b, it should come
+        *      before b.
+        */
+       if (!da_a && da_b) return +1;
+       if (da_a && !da_b) return -1;
+
+       if (vp_a->tag > vp_b->tag) return +1;
+       if (vp_b->tag < vp_b->tag) return -1;
+
+       return 0;
+}
+
 /** Compare two pairs, using the operator from "a"
  *
  *     i.e. given two attributes, it does:

diff --git a/src/modules/proto_dhcp/dhcp.c b/src/modules/proto_dhcp/dhcp.c
index 46f99179aff6c6112e0e37951caf6f4b37147c25..3e177951a50bd39d069aa07bfc49e7ddbe70fe9e 100644 (file)
--- a/src/modules/proto_dhcp/dhcp.c
+++ b/src/modules/proto_dhcp/dhcp.c
@@ -1323,18 +1323,28 @@ int8_t fr_dhcp_attr_cmp(void const *a, void const *b)
        VERIFY_VP(my_b);
 
        /*
-        *      DHCP-Message-Type is first, for simplicity.
+        *      We can only use attribute numbers if we know they're
+        *      not nested attributes.
+        *
+        *      @fixme We should be able to use my_a->da->parent->flags.is_root,
+        *      but the DHCP attributes are hacked into the server under a vendor
+        *      dictionary, so we can't.
         */
-       if ((my_a->da->attr == PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr != PW_DHCP_MESSAGE_TYPE)) return -1;
+       if ((my_a->da->parent->type != PW_TYPE_TLV) && (my_b->da->parent->type != PW_TYPE_TLV)) {
+               /*
+                *      DHCP-Message-Type is first, for simplicity.
+                */
+               if ((my_a->da->attr == PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr != PW_DHCP_MESSAGE_TYPE)) return -1;
+               if ((my_a->da->attr != PW_DHCP_MESSAGE_TYPE) && (my_b->da->attr == PW_DHCP_MESSAGE_TYPE)) return +1;
 
-       /*
-        *      Relay-Agent is last
-        */
-       if ((my_a->da->attr == PW_DHCP_OPTION_82) && (my_b->da->attr != PW_DHCP_OPTION_82)) return 1;
-       if (my_a->da->attr < my_b->da->attr) return -1;
-       if (my_a->da->attr > my_b->da->attr) return 1;
+               /*
+                *      Relay-Agent is last
+                */
+               if ((my_a->da->attr == PW_DHCP_OPTION_82) && (my_b->da->attr != PW_DHCP_OPTION_82)) return +1;
+               if ((my_a->da->attr != PW_DHCP_OPTION_82) && (my_b->da->attr == PW_DHCP_OPTION_82)) return -1;
+       }
 
-       return 0;
+       return fr_pair_cmp_by_parent_num_tag(my_a->da, my_b->da);
 }
 
 /** Write DHCP option value into buffer