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

[Dave Reisner]

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.gregor...@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
> 
>