time: Have our time_point and sign the difference too

Make operator - between Ceph time points always return a signed duration.

This fixes the underflow pointed out by Kefu Chai <kchai@redhat.com>.

Signed-off-by: Adam C. Emerson <aemerson@redhat.com>

diff --git a/src/common/ceph_time.h b/src/common/ceph_time.h
index 6a398fa243946e918e57191a14dcdb555619605f..70b30a9dda41e86973e0bf0ee476219565bc8722 100644 (file)
--- a/src/common/ceph_time.h
+++ b/src/common/ceph_time.h
@@ -263,6 +263,44 @@ namespace ceph {
        return time_point(seconds(ts.tv_sec) + nanoseconds(ts.tv_nsec));
       }
     };
+
+    // So that our subtractions produce negative spans rather than
+    // arithmetic underflow.
+    namespace {
+      template<typename Rep1, typename Period1, typename Rep2,
+              typename Period2>
+      inline auto difference(std::chrono::duration<Rep1, Period1> minuend,
+                            std::chrono::duration<Rep2, Period2> subtrahend)
+       -> typename std::common_type<
+         std::chrono::duration<typename std::make_signed<Rep1>::type,
+                               Period1>,
+         std::chrono::duration<typename std::make_signed<Rep2>::type,
+                               Period2> >::type {
+       // Foo.
+       using srep =
+         typename std::common_type<
+           std::chrono::duration<typename std::make_signed<Rep1>::type,
+                                 Period1>,
+           std::chrono::duration<typename std::make_signed<Rep2>::type,
+                                 Period2> >::type;
+       return srep(srep(minuend).count() - srep(subtrahend).count());
+      }
+
+      template<typename Clock, typename Duration1, typename Duration2>
+      inline auto difference(
+       typename std::chrono::time_point<Clock, Duration1> minuend,
+       typename std::chrono::time_point<Clock, Duration2> subtrahend)
+       -> typename std::common_type<
+         std::chrono::duration<typename std::make_signed<
+                                 typename Duration1::rep>::type,
+                               typename Duration1::period>,
+         std::chrono::duration<typename std::make_signed<
+                                 typename Duration2::rep>::type,
+                               typename Duration2::period> >::type {
+       return difference(minuend.time_since_epoch(),
+                         subtrahend.time_since_epoch());
+      }
+    }
   } // namespace time_detail
 
   // duration is the concrete time representation for our code in the
@@ -357,16 +395,46 @@ namespace ceph {
          ceil(timepoint.time_since_epoch(), precision));
   }
 
-  static inline timespan make_timespan(const double d) {
-    return std::chrono::duration_cast<timespan>(
-      std::chrono::duration<double>(d));
+  namespace {
+    inline timespan make_timespan(const double d) {
+      return std::chrono::duration_cast<timespan>(
+       std::chrono::duration<double>(d));
+    }
   }
 
   std::ostream& operator<<(std::ostream& m, const timespan& t);
   std::ostream& operator<<(std::ostream& m, const real_time& t);
   std::ostream& operator<<(std::ostream& m, const mono_time& t);
 
-  // To gain sane behavior with signs
+  // The way std::chrono handles the return type of subtraction is not
+  // wonderful. The difference of two unsigned types SHOULD be signed.
+
+  namespace {
+    inline signedspan operator -(real_time minuend,
+                                real_time subtrahend) {
+      return time_detail::difference(minuend, subtrahend);
+    }
+
+    inline signedspan operator -(coarse_real_time minuend,
+                                coarse_real_time subtrahend) {
+      return time_detail::difference(minuend, subtrahend);
+    }
+
+    inline signedspan operator -(mono_time minuend,
+                                mono_time subtrahend) {
+      return time_detail::difference(minuend, subtrahend);
+    }
+
+    inline signedspan operator -(coarse_mono_time minuend,
+                                coarse_mono_time subtrahend) {
+      return time_detail::difference(minuend, subtrahend);
+    }
+  }
+
+  // We could add specializations of time_point - duration and
+  // time_point + duration to assert on overflow, but I don't think we
+  // should.
+
 } // namespace ceph
 
 // We need these definitions to be able to hande ::encode/::decode on
@@ -411,6 +479,15 @@ namespace std {
       ::decode(t, p);
     }
   } // namespace chrono
+
+  // An overload of our own
+  namespace {
+    inline timespan abs(signedspan z) {
+      return z > signedspan::zero() ?
+       std::chrono::duration_cast<timespan>(z) :
+       timespan(-z.count());
+    }
+  }
 } // namespace std
 
 #endif // COMMON_CEPH_TIME_H

diff --git a/src/test/common/test_time.cc b/src/test/common/test_time.cc
index a54fe90b83a3af596581e60b624d2c838f793a8d..944492031ff78602c8540ef5aaa70524d6f4ffa3 100644 (file)
--- a/src/test/common/test_time.cc
+++ b/src/test/common/test_time.cc
@@ -122,9 +122,7 @@ static void system_clock_conversions() {
 
   ASSERT_EQ(Clock::to_double(brt), bd);
   // Fudge factor
-  ASSERT_LT((Clock::from_double(bd) >  brt ?
-            Clock::from_double(bd) - brt :
-            brt - Clock::from_double(bd)).count(), 30U);
+  ASSERT_LT(std::abs((Clock::from_double(bd) - brt).count()), 30);
 }
 
 TEST(RealClock, Sanity) {
@@ -144,3 +142,37 @@ TEST(CoarseRealClock, Sanity) {
 TEST(CoarseRealClock, Conversions) {
   system_clock_conversions<coarse_real_clock>();
 }
+
+TEST(TimePoints, SignedSubtraciton) {
+  ceph::real_time rta(std::chrono::seconds(3));
+  ceph::real_time rtb(std::chrono::seconds(5));
+
+  ceph::coarse_real_time crta(std::chrono::seconds(3));
+  ceph::coarse_real_time crtb(std::chrono::seconds(5));
+
+  ceph::mono_time mta(std::chrono::seconds(3));
+  ceph::mono_time mtb(std::chrono::seconds(5));
+
+  ceph::coarse_mono_time cmta(std::chrono::seconds(3));
+  ceph::coarse_mono_time cmtb(std::chrono::seconds(5));
+
+  ASSERT_LT(rta - rtb, ceph::signedspan::zero());
+  ASSERT_LT((rta - rtb).count(), 0);
+  ASSERT_GT(rtb - rta, ceph::signedspan::zero());
+  ASSERT_GT((rtb - rta).count(), 0);
+
+  ASSERT_LT(crta - crtb, ceph::signedspan::zero());
+  ASSERT_LT((crta - crtb).count(), 0);
+  ASSERT_GT(crtb - crta, ceph::signedspan::zero());
+  ASSERT_GT((crtb - crta).count(), 0);
+
+  ASSERT_LT(mta - mtb, ceph::signedspan::zero());
+  ASSERT_LT((mta - mtb).count(), 0);
+  ASSERT_GT(mtb - mta, ceph::signedspan::zero());
+  ASSERT_GT((mtb - mta).count(), 0);
+
+  ASSERT_LT(cmta - cmtb, ceph::signedspan::zero());
+  ASSERT_LT((cmta - cmtb).count(), 0);
+  ASSERT_GT(cmtb - cmta, ceph::signedspan::zero());
+  ASSERT_GT((cmtb - cmta).count(), 0);
+}