rplan: fix kr_qflags_*() to work with more than 32 flags

Originally division around sizeof(uint32_t) caused silent truncation
for struct kr_qflags with sizes not multiple of 4 bytes.

Attempts to optimize using uint32_t blocks could lead to read/write
beyond end of uint32_t so I'm not willing to risk it.

Also, the code was refactored to avoid duplication between _set and _clear.
Quick look into assembly produced by gcc 7.2.1 with -O2 on x86_64 confirms that
all auxiliary functions got inlined so there are not extra function calls.

Unit tests are attached. These fail on the previous version of _set() and
_clear() and work now.

diff --git a/lib/rplan.c b/lib/rplan.c
index 6bdd87c9d892ec1e4eed5d940326f669391a7b8f..c4a327992312699d4045c3fff507c4a80bda4c51 100644 (file)
--- a/lib/rplan.c
+++ b/lib/rplan.c
@@ -27,37 +27,58 @@
 #define QUERY_PROVIDES(q, name, cls, type) \
     ((q)->sclass == (cls) && (q)->stype == type && knot_dname_is_equal((q)->sname, name))
 
-void kr_qflags_set(struct kr_qflags *fl1, struct kr_qflags fl2)
+inline static unsigned char chars_or(const unsigned char a, const unsigned char b)
+{
+       return a | b;
+}
+
+/** Bits set to 1 in variable b will be set to zero in variable a. */
+inline static unsigned char chars_mask(const unsigned char a, const unsigned char b)
+{
+       return a & ~b;
+}
+
+/** Apply mod(a, b) to every byte a, b from fl1, fl2 and return result in fl1. */
+inline static void kr_qflags_mod(struct kr_qflags *fl1, struct kr_qflags fl2,
+                       unsigned char mod(const unsigned char a, const unsigned char b))
 {
        if (!fl1) abort();
-       typedef uint32_t ui;
        union {
                struct kr_qflags flags;
-               ui uints[sizeof(struct kr_qflags) / sizeof(ui)];
+               /* C99 section 6.5.3.4: sizeof(char) == 1 */
+               unsigned char chars[sizeof(struct kr_qflags)];
        } tmp1, tmp2;
        /* The compiler should be able to optimize all this into simple ORs. */
        tmp1.flags = *fl1;
        tmp2.flags = fl2;
-       for (size_t i = 0; i < sizeof(tmp1.uints) / sizeof(tmp1.uints[0]); ++i) {
-               tmp1.uints[i] |= tmp2.uints[i];
+       for (size_t i = 0; i < sizeof(struct kr_qflags); ++i) {
+               tmp1.chars[i] = mod(tmp1.chars[i], tmp2.chars[i]);
        }
        *fl1 = tmp1.flags;
 }
+
+/**
+ * Set bits from variable fl2 in variable fl1.
+ * Bits which are not set in fl2 are not modified in fl1.
+ *
+ * @param[in,out] fl1
+ * @param[in] fl2
+ */
+void kr_qflags_set(struct kr_qflags *fl1, struct kr_qflags fl2)
+{
+       kr_qflags_mod(fl1, fl2, chars_or);
+}
+
+/**
+ * Clear bits from variable fl2 in variable fl1.
+ * Bits which are not set in fl2 are not modified in fl1.
+ *
+ * @param[in,out] fl1
+ * @param[in] fl2
+ */
 void kr_qflags_clear(struct kr_qflags *fl1, struct kr_qflags fl2)
 {
-       if (!fl1) abort();
-       typedef uint32_t ui;
-       union {
-               struct kr_qflags flags;
-               ui uints[sizeof(struct kr_qflags) / sizeof(ui)];
-       } tmp1, tmp2;
-       /* The compiler should be able to optimize all this into simple ORs. */
-       tmp1.flags = *fl1;
-       tmp2.flags = fl2;
-       for (size_t i = 0; i < sizeof(tmp1.uints) / sizeof(tmp1.uints[0]); ++i) {
-               tmp1.uints[i] &= ~tmp2.uints[i];
-       }
-       *fl1 = tmp1.flags;
+       kr_qflags_mod(fl1, fl2, chars_mask);
 }
 
 static struct kr_query *query_create(knot_mm_t *pool, const knot_dname_t *name, uint32_t uid)

diff --git a/tests/test_rplan.c b/tests/test_rplan.c
index 08c6611aaee5272a55355e4a23059755a4910665..ad083fd48aa9311d7f0eb2d009b555dbc3cb6ab4 100644 (file)
--- a/tests/test_rplan.c
+++ b/tests/test_rplan.c
@@ -56,11 +56,31 @@ static void test_rplan_push(void **state)
        kr_rplan_deinit(&rplan);
 }
 
+/**
+ * Set and clear must not omit any bit, especially around byte boundaries.
+ */
+static void test_rplan_flags(void **state)
+{
+       static struct kr_qflags f1, f2, ones, zeros; /* static => initialized to zeroes */
+       assert_true(memcmp(&f1, &f2, sizeof(f1)) == 0); /* sanity check */
+       memset(&ones, 0xff, sizeof(ones)); /* all ones */
+
+       /* test set */
+       kr_qflags_set(&f1, ones);
+       assert_true(memcmp(&f1, &ones, sizeof(f1)) == 0);  /* 1 == 1 */
+
+       /* test clear */
+       memset(&f2, 0xff, sizeof(f2)); /* all ones */
+       kr_qflags_clear(&f2, ones);
+       assert_true(memcmp(&f2, &zeros, sizeof(f1)) == 0);  /* 0 == 0 */
+}
+
 int main(void)
 {
        const UnitTest tests[] = {
                unit_test(test_rplan_params),
-               unit_test(test_rplan_push)
+               unit_test(test_rplan_push),
+               unit_test(test_rplan_flags)
        };
 
        return run_tests(tests);