Re: [pacman-dev] [PATCHv2] use monotonic clock for progress updates

On Sat, Jun 28, 2014 at 12:07:03PM -0400, Andrew Gregory wrote:

gettimeofday is susceptible to system time adjustments, skewing or altogether breaking progress output. For the sake of platforms that lack clock_gettime support, gettimeofday is retained as a fallback.

Note that CLOCK_MONOTONIC is still susceptible to skew from adjtime (called by {S,}NTP).

Fixes FS#36983

Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com

src/pacman/callback.c | 61 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 23 deletions(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 9cde1de..dafaeaf 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -21,7 +21,6 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/time.h> #include <sys/types.h> /* off_t */ #include <time.h> #include <unistd.h> @@ -47,6 +46,33 @@ static alpm_list_t *output = NULL; /* update speed for the fill_progress based functions */ #define UPDATE_SPEED_MS 200

+#if defined(CLOCK_MONOTONIC) && defined(_POSIX_MONOTONIC_CLOCK)

+static void get_time(struct timespec *ts) +{

• clock_gettime(CLOCK_MONOTONIC, ts);

+}

+#else

+/* darwin doesn't support clock_gettime, fallback to gettimeofday */ +#include <sys/time.h> +static void get_time(struct timespec *ts)

This var is forever unused and will likely throw a compile-time error with our flags. Is there any case where we ever actually care about the underlying struct? Why not just return an int64_t or double, e.g.

https://github.com/falconindy/pkgfile/blob/master/src/update.c#L53

+{

• struct timeval tv;
• gettimeofday(&tv, NULL);
• ts->tv_sec = tv->tv_sec;
• ts->tv_nsec = tv->tv_usec * 1000;

+}

+#endif

+static long diff_timespec_ms(struct timespec *t1, struct timespec *t0) {

• time_t diff_sec = t1->tv_sec - t0->tv_sec;
  

• long diff_nsec = t1->tv_nsec - t0->tv_nsec;
  

• return (diff_sec * 1000) + (diff_nsec / 1000000);
  

+}

/**

• Silly little helper function, determines if the caller needs a visual update
• since the last time this function was called.

@@ -57,24 +83,18 @@ static alpm_list_t *output = NULL; static long get_update_timediff(int first_call) { long retval = 0;

• static struct timeval last_time = {0, 0};

• static struct timespec last_time = {0, 0};

  /* on first call, simply set the last time and return */ if(first_call) {

• gettimeofday(&last_time, NULL);
  

• get_time(&last_time);
  

  } else {

• struct timeval this_time;
  

• time_t diff_sec;
  

• suseconds_t diff_usec;
  

• gettimeofday(&this_time, NULL);
  

• diff_sec = this_time.tv_sec - last_time.tv_sec;
  

• diff_usec = this_time.tv_usec - last_time.tv_usec;
  

• retval = (diff_sec * 1000) + (diff_usec / 1000);
  

• struct timespec this_time;
  

• get_time(&this_time);
  

• retval = diff_timespec_ms(&this_time, &last_time);
  
  

  /* do not update last_time if interval was too short */

• if(retval >= UPDATE_SPEED_MS) {
  

• if(retval < 0 || retval >= UPDATE_SPEED_MS) {
  last_time = this_time;
  

  } }

@@ -647,7 +667,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) { static double rate_last; static off_t xfered_last;

• static struct timeval initial_time;

• static struct timespec initial_time; int infolen; int filenamelen; char *fname, *p;

@@ -709,21 +729,16 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) /* set default starting values, ensure we only call this once * if TotalDownload is enabled */ if(!totaldownload || (totaldownload && list_xfered == 0)) {

• 	gettimeofday(&initial_time, NULL);
  

• 	get_time(&initial_time);
  xfered_last = (off_t)0;
  rate_last = 0.0;
  get_update_timediff(1);
  

  } } else if(file_xfered == file_total) { /* compute final values */

• struct timeval current_time;
  

• time_t diff_sec;
  

• suseconds_t diff_usec;
  

• gettimeofday(&current_time, NULL);
  

• diff_sec = current_time.tv_sec - initial_time.tv_sec;
  

• diff_usec = current_time.tv_usec - initial_time.tv_usec;
  

• timediff = (diff_sec * 1000) + (diff_usec / 1000);
  

• struct timespec current_time;
  

• get_time(&current_time);
  

• timediff = diff_timespec_ms(&current_time, &initial_time);
  

  if(timediff > 0) { rate = (double)xfered / (timediff / 1000.0); /* round elapsed time (in ms) to the nearest second */

-- 2.0.1