[1278] address review comments

src/lib/dns/python/serial_python.cc

@@ -144,8 +144,7 @@ Serial_richcmp(s_Serial* self, s_Serial* other, int op) {
         c = *self->cppobj < *other->cppobj;
         break;
     case Py_LE:
-        c = *self->cppobj < *other->cppobj ||
-            *self->cppobj == *other->cppobj;
+        c = *self->cppobj <= *other->cppobj;
         break;
     case Py_EQ:
         c = *self->cppobj == *other->cppobj;
@@ -154,11 +153,10 @@ Serial_richcmp(s_Serial* self, s_Serial* other, int op) {
         c = *self->cppobj != *other->cppobj;
         break;
     case Py_GT:
-        c = *other->cppobj < *self->cppobj;
+        c = *self->cppobj > *other->cppobj;
         break;
     case Py_GE:
-        c = *other->cppobj < *self->cppobj ||
-            *self->cppobj == *other->cppobj;
+        c = *self->cppobj >= *other->cppobj;
         break;
     }
     if (c) {
@@ -222,7 +220,7 @@ PyTypeObject serial_type = {
     "main purpose of this class is to provide serial number arithmetic, as "
     "described in RFC 1892. Objects of this type can be compared and added "
     "to each other, as described in RFC 1892. Apart from str(), get_value(), "
-    "comparison operators, and the + operand, no other operations are "
+    "comparison operators, and the + operator, no other operations are "
     "defined for this type.",
     NULL,                               // tp_traverse
     NULL,                               // tp_clear

src/lib/dns/python/tests/serial_python_test.py

@@ -100,5 +100,10 @@ class SerialTest(unittest.TestCase):
         self.assertEqual(100 + self.one, self.highest + 102)
         self.assertEqual(self.zero + 2147483645, self.highest + 2147483646)
 
+        # using lambda so the error doesn't get thrown on initial evaluation
+        self.assertRaises(TypeError, lambda: self.zero + "bad")
+        self.assertRaises(TypeError, lambda: self.zero + None)
+        self.assertRaises(TypeError, lambda: "bad" + self.zero)
+
 if __name__ == '__main__':
     unittest.main()

src/lib/dns/serial.cc

@@ -32,9 +32,9 @@ Serial::operator<(const Serial& other) const {
     uint32_t other_val = other.getValue();
     bool result = false;
     if (value_ < other_val) {
-        result = ((other_val - value_) <= MAX_INCREMENT);
+        result = ((other_val - value_) <= MAX_SERIAL_INCREMENT);
     } else if (other_val < value_) {
-        result = ((value_ - other_val) > MAX_INCREMENT);
+        result = ((value_ - other_val) > MAX_SERIAL_INCREMENT);
     }
     return (result);
 }
@@ -56,21 +56,9 @@ Serial::operator>=(const Serial& other) const {
 
 Serial
 Serial::operator+(uint32_t other_val) const {
-    if (value_ <= MAX_INCREMENT) {
-        // just add
-        return (Serial(value_ + other_val));
-    } else {
-        // check whether it wouldn't wrap
-        // Note the +1 here and the -2 below, these operations use
-        // the MAX_INCREMENT constant, which is 2^31-1, while it really
-        // needs 2^31 itself.
-        if (value_ - MAX_INCREMENT + other_val <= MAX_INCREMENT + 1) {
-            return (Serial(value_ + other_val));
-        } else {
-            return (Serial(value_ - MAX_INCREMENT +
-                           other_val - MAX_INCREMENT - 2));
-        }
-    }
+    uint64_t new_val = static_cast<uint64_t>(value_) +
+                       static_cast<uint64_t>(other_val);
+    return Serial(static_cast<uint32_t>(new_val % MAX_SERIAL_VALUE));
 }
 
 Serial

src/lib/dns/serial.h

@@ -24,7 +24,10 @@ namespace dns {
 /// The maximum difference between two serial numbers. If the (plain uint32_t)
 /// difference between two serials is greater than this number, the smaller one
 /// is considered greater.
-const uint32_t MAX_INCREMENT = 2147483647;
+const uint32_t MAX_SERIAL_INCREMENT = 2147483647;
+
+/// Maximum value a serial can have, used in + operator.
+const uint64_t MAX_SERIAL_VALUE = 4294967296;
 
 /// \brief This class defines DNS serial numbers and serial arithmetic.
 ///
@@ -115,7 +118,7 @@ public:
     /// \brief Adds the given value to the serial number. If this would make
     /// the number greater than 2^32-1, it is 'wrapped'.
     /// \note According to the specification, an addition greater than
-    /// MAX_INCREMENT is undefined. We do NOT catch this error (so as not
+    /// MAX_SERIAL_INCREMENT is undefined. We do NOT catch this error (so as not
     /// to raise exceptions), but this behaviour remains undefined.
     ///
     /// \param other The Serial to add
@@ -127,7 +130,7 @@ public:
     /// the number greater than 2^32-1, it is 'wrapped'.
     ///
     /// \note According to the specification, an addition greater than
-    /// MAX_INCREMENT is undefined. We do NOT catch this error (so as not
+    /// MAX_SERIAL_INCREMENT is undefined. We do NOT catch this error (so as not
     /// to raise exceptions), but this behaviour remains undefined.
     ///
     /// \param other_val The uint32_t value to add

src/lib/dns/tests/serial_unittest.cc

@@ -88,7 +88,7 @@ TEST_F(SerialTest, addition) {
 
     EXPECT_EQ(one + 100, max + 102);
     EXPECT_EQ(min + 2147483645, max + 2147483646);
-    EXPECT_EQ(min + 2147483646, max + MAX_INCREMENT);
+    EXPECT_EQ(min + 2147483646, max + MAX_SERIAL_INCREMENT);
 }
 
 //
@@ -109,8 +109,8 @@ void do_addition_larger_test(const Serial& number) {
     EXPECT_GT(number + 100, number);
     EXPECT_GT(number + 1111111, number);
     EXPECT_GT(number + 2147483646, number);
-    EXPECT_GT(number + MAX_INCREMENT, number);
-    // Try MAX_INCREMENT as a hardcoded number as well
+    EXPECT_GT(number + MAX_SERIAL_INCREMENT, number);
+    // Try MAX_SERIAL_INCREMENT as a hardcoded number as well
     EXPECT_GT(number + 2147483647, number);
 }
 
@@ -142,7 +142,7 @@ do_two_additions_test_second(const Serial &original,
     EXPECT_NE(original, number + 100);
     EXPECT_NE(original, number + 1111111);
     EXPECT_NE(original, number + 2147483646);
-    EXPECT_NE(original, number + MAX_INCREMENT);
+    EXPECT_NE(original, number + MAX_SERIAL_INCREMENT);
     EXPECT_NE(original, number + 2147483647);
 }
 
@@ -152,7 +152,7 @@ void do_two_additions_test_first(const Serial &number) {
     do_two_additions_test_second(number, number + 100);
     do_two_additions_test_second(number, number + 1111111);
     do_two_additions_test_second(number, number + 2147483646);
-    do_two_additions_test_second(number, number + MAX_INCREMENT);
+    do_two_additions_test_second(number, number + MAX_SERIAL_INCREMENT);
     do_two_additions_test_second(number, number + 2147483647);
 }