diff mbox series

[ovs-dev,v2,1/2] timeval: Introduce macros to convert timespec and timeval.

Message ID 1510692151-14322-1-git-send-email-bhanuprakash.bodireddy@intel.com
State Superseded
Headers show
Series [ovs-dev,v2,1/2] timeval: Introduce macros to convert timespec and timeval. | expand

Commit Message

Bodireddy, Bhanuprakash Nov. 14, 2017, 8:42 p.m. UTC 
This commit replaces the numbers with MSEC_PER_SEC, NSEC_PER_SEC and
USEC_PER_MSEC macros when dealing with timespec and timeval.

This commit doesn't change functionality.

Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
---
 lib/timeval.c                | 29 ++++++++++++++++-------------
 lib/timeval.h                |  7 +++++++
 lib/util.c                   |  6 +++---
 ofproto/ofproto-dpif-ipfix.c |  2 +-
 4 files changed, 27 insertions(+), 17 deletions(-)

Comments

Ben Pfaff Nov. 28, 2017, 8:47 p.m. UTC | #1 
On Tue, Nov 14, 2017 at 08:42:30PM +0000, Bhanuprakash Bodireddy wrote:
> This commit replaces the numbers with MSEC_PER_SEC, NSEC_PER_SEC and
> USEC_PER_MSEC macros when dealing with timespec and timeval.
> 
> This commit doesn't change functionality.
> 
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>

This still seems careless and risky to me.

For example:
    msecs = secs * MSEC_PER_SEC * 1LL;
which expands to
    msecs = secs * 1000L * 1LL;
still risks overflow on a 32-bit system (where 1000L is 32 bits long).

The previous version of the code didn't have that problem:
    msecs = secs * 1000LL;

Maybe it would be better to just leave these as-is.
Bodireddy, Bhanuprakash Nov. 28, 2017, 10:18 p.m. UTC | #2 
Hi Ben,

>On Tue, Nov 14, 2017 at 08:42:30PM +0000, Bhanuprakash Bodireddy wrote:
>> This commit replaces the numbers with MSEC_PER_SEC, NSEC_PER_SEC and
>> USEC_PER_MSEC macros when dealing with timespec and timeval.
>>
>> This commit doesn't change functionality.
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> <bhanuprakash.bodireddy@intel.com>
>
>This still seems careless and risky to me.
>
>For example:
>    msecs = secs * MSEC_PER_SEC * 1LL;
>which expands to
>    msecs = secs * 1000L * 1LL;
>still risks overflow on a 32-bit system (where 1000L is 32 bits long).
>
>The previous version of the code didn't have that problem:
>    msecs = secs * 1000LL;
>
>Maybe it would be better to just leave these as-is.

I agree with you and take back my changes w.r.t introducing the time MACROS. I have posted v3 version replacing the Macro.
On an unrelated note, can you please also review the patch here that extends get_process_info(). 
           https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/340762.html

My Keepalive patch series has dependency on high resolution timer patch and above mentioned API.
 
- Bhanuprakash.
Ben Pfaff Nov. 28, 2017, 11:51 p.m. UTC | #3 
On Tue, Nov 28, 2017 at 10:18:30PM +0000, Bodireddy, Bhanuprakash wrote:
> Hi Ben,
> 
> >On Tue, Nov 14, 2017 at 08:42:30PM +0000, Bhanuprakash Bodireddy wrote:
> >> This commit replaces the numbers with MSEC_PER_SEC, NSEC_PER_SEC and
> >> USEC_PER_MSEC macros when dealing with timespec and timeval.
> >>
> >> This commit doesn't change functionality.
> >>
> >> Signed-off-by: Bhanuprakash Bodireddy
> >> <bhanuprakash.bodireddy@intel.com>
> >
> >This still seems careless and risky to me.
> >
> >For example:
> >    msecs = secs * MSEC_PER_SEC * 1LL;
> >which expands to
> >    msecs = secs * 1000L * 1LL;
> >still risks overflow on a 32-bit system (where 1000L is 32 bits long).
> >
> >The previous version of the code didn't have that problem:
> >    msecs = secs * 1000LL;
> >
> >Maybe it would be better to just leave these as-is.
> 
> I agree with you and take back my changes w.r.t introducing the time MACROS. I have posted v3 version replacing the Macro.
> On an unrelated note, can you please also review the patch here that extends get_process_info(). 
>            https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/340762.html
> 
> My Keepalive patch series has dependency on high resolution timer patch and above mentioned API.

OK.

Thank you for your patience with my comments on this series.  I will
review v3 now.
diff mbox series

Patch

diff --git a/lib/timeval.c b/lib/timeval.c
index b60bf30..567c26e 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -266,7 +266,7 @@  time_alarm(unsigned int secs)
     time_init();
 
     now = time_msec();
-    msecs = secs * 1000LL;
+    msecs = secs * MSEC_PER_SEC * 1LL;
     deadline = now < LLONG_MAX - msecs ? now + msecs : LLONG_MAX;
 }
 
@@ -372,25 +372,28 @@  time_poll(struct pollfd *pollfds, int n_pollfds, HANDLE *handles OVS_UNUSED,
 long long int
 timespec_to_msec(const struct timespec *ts)
 {
-    return (long long int) ts->tv_sec * 1000 + ts->tv_nsec / (1000 * 1000);
+    return (long long int) ts->tv_sec * MSEC_PER_SEC +
+            ts->tv_nsec / NSEC_PER_MSEC;
 }
 
 long long int
 timeval_to_msec(const struct timeval *tv)
 {
-    return (long long int) tv->tv_sec * 1000 + tv->tv_usec / 1000;
+    return (long long int) tv->tv_sec * MSEC_PER_SEC +
+            tv->tv_usec / USEC_PER_MSEC;
 }
 
 long long int
 timespec_to_usec(const struct timespec *ts)
 {
-    return (long long int) ts->tv_sec * 1000 * 1000 + ts->tv_nsec / 1000;
+    return (long long int) ts->tv_sec * USEC_PER_SEC +
+            ts->tv_nsec / NSEC_PER_USEC;
 }
 
 long long int
 timeval_to_usec(const struct timeval *tv)
 {
-    return (long long int) tv->tv_sec * 1000 * 1000 + tv->tv_usec;
+    return (long long int) tv->tv_sec * USEC_PER_SEC + tv->tv_usec;
 }
 
 /* Returns the monotonic time at which the "time" module was initialized, in
@@ -510,8 +513,8 @@  xclock_gettime(clock_t id, struct timespec *ts)
 static void
 msec_to_timespec(long long int ms, struct timespec *ts)
 {
-    ts->tv_sec = ms / 1000;
-    ts->tv_nsec = (ms % 1000) * 1000 * 1000;
+    ts->tv_sec = ms / MSEC_PER_SEC;
+    ts->tv_nsec = (ms % 1000) * NSEC_PER_MSEC;
 }
 
 static void
@@ -596,8 +599,8 @@  timespec_add(struct timespec *sum,
 
     tmp.tv_sec = a->tv_sec + b->tv_sec;
     tmp.tv_nsec = a->tv_nsec + b->tv_nsec;
-    if (tmp.tv_nsec >= 1000 * 1000 * 1000) {
-        tmp.tv_nsec -= 1000 * 1000 * 1000;
+    if (tmp.tv_nsec >= NSEC_PER_SEC) {
+        tmp.tv_nsec -= NSEC_PER_SEC;
         tmp.tv_sec++;
     }
 
@@ -621,7 +624,7 @@  log_poll_interval(long long int last_wakeup)
 {
     long long int interval = time_msec() - last_wakeup;
 
-    if (interval >= 1000 && !is_warped(&monotonic_clock)) {
+    if (interval >= MSEC_PER_SEC && !is_warped(&monotonic_clock)) {
         const struct rusage *last_rusage = get_recent_rusage();
         struct rusage rusage;
 
@@ -713,7 +716,7 @@  refresh_rusage(void)
 
     if (!getrusage_thread(recent_rusage)) {
         long long int now = time_msec();
-        if (now >= t->newer.when + 3 * 1000) {
+        if (now >= t->newer.when + 3 * MSEC_PER_SEC) {
             t->older = t->newer;
             t->newer.when = now;
             t->newer.cpu = (timeval_to_msec(&recent_rusage->ru_utime) +
@@ -837,7 +840,7 @@  strftime_msec(char *s, size_t max, const char *format,
 struct tm_msec *
 localtime_msec(long long int now, struct tm_msec *result)
 {
-  time_t now_sec = now / 1000;
+  time_t now_sec = now / MSEC_PER_SEC;
   localtime_r(&now_sec, &result->tm);
   result->msec = now % 1000;
   return result;
@@ -846,7 +849,7 @@  localtime_msec(long long int now, struct tm_msec *result)
 struct tm_msec *
 gmtime_msec(long long int now, struct tm_msec *result)
 {
-  time_t now_sec = now / 1000;
+  time_t now_sec = now / MSEC_PER_SEC;
   gmtime_r(&now_sec, &result->tm);
   result->msec = now % 1000;
   return result;
diff --git a/lib/timeval.h b/lib/timeval.h
index c3dbb51..5e2a731 100644
--- a/lib/timeval.h
+++ b/lib/timeval.h
@@ -40,6 +40,13 @@  BUILD_ASSERT_DECL(TYPE_IS_SIGNED(time_t));
 #define TIME_MAX TYPE_MAXIMUM(time_t)
 #define TIME_MIN TYPE_MINIMUM(time_t)
 
+#define MSEC_PER_SEC    1000L
+#define USEC_PER_SEC    1000000L
+#define NSEC_PER_SEC    1000000000L
+#define USEC_PER_MSEC   1000L
+#define NSEC_PER_MSEC   1000000L
+#define NSEC_PER_USEC   1000L
+
 #ifdef _WIN32
 #define localtime_r(timep, result) localtime_s(result, timep)
 #define gmtime_r(timep, result) gmtime_s(result, timep)
diff --git a/lib/util.c b/lib/util.c
index 9e6edd2..17c2c99 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -586,7 +586,7 @@  get_boot_time(void)
         char line[128];
         FILE *stream;
 
-        cache_expiration = time_msec() + 5 * 1000;
+        cache_expiration = time_msec() + 5 * MSEC_PER_SEC;
 
         stream = fopen(stat_file, "r");
         if (!stream) {
@@ -598,7 +598,7 @@  get_boot_time(void)
         while (fgets(line, sizeof line, stream)) {
             long long int btime;
             if (ovs_scan(line, "btime %lld", &btime)) {
-                boot_time = btime * 1000;
+                boot_time = btime * MSEC_PER_SEC;
                 goto done;
             }
         }
@@ -2198,7 +2198,7 @@  xsleep(unsigned int seconds)
 {
     ovsrcu_quiesce_start();
 #ifdef _WIN32
-    Sleep(seconds * 1000);
+    Sleep(seconds * MSEC_PER_SEC);
 #else
     sleep(seconds);
 #endif
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 4d16878..dda7657 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -1869,7 +1869,7 @@  ipfix_update_stats(struct dpif_ipfix_exporter *exporter,
 static uint64_t
 ipfix_now(void)
 {
-    return time_wall_msec() * 1000ULL;
+    return time_wall_usec();
 }
 
 /* Add an entry into a flow cache.  The entry is either aggregated into