On 7 December 2013 05:32, Andrew Gregory wrote:

Previously, if the system time was adjusted backwards during a progress display, get_update_timediff return negative values prevent the progress bar from updating. Instead, on negative values, the saved time is reset. Additionally, the download callback ignores the amount downloaded to avoid skewing the rate.

Have you considered using clock_gettime(CLOCK_MONOTONIC) rather than gettimeofday()? Or is it not available on some platforms? Even if you have to use a different function for OS X or something it mightn’t be much worse than these backwards time hacks.

Fixes FS#36983

Signed-off-by: Andrew Gregory --- src/pacman/callback.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/pacman/callback.c b/src/pacman/callback.c index a181fa5..0ffb2ef 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -73,8 +73,8 @@ static long get_update_timediff(int first_call)

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

- /* do not update last_time if interval was too short */ - if(retval >= UPDATE_SPEED_MS) { + /* update last_time if it is in the future or interval was long enough */ + if(retval < 0 || retval >= UPDATE_SPEED_MS) { last_time = this_time; } } @@ -554,7 +554,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 long elapsed_time = 0; int infolen; int filenamelen; char *fname, *p; @@ -616,25 +616,23 @@ 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); + elapsed_time = 0; 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; + timediff = get_update_timediff(0);

- gettimeofday(¤t_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); if(timediff > 0) { - rate = (double)xfered / (timediff / 1000.0); + elapsed_time += timediff; + } + + if(elapsed_time > 0) { + rate = (double)xfered / (elapsed_time / 1000.0); /* round elapsed time (in ms) to the nearest second */ - eta_s = (unsigned int)(timediff + 500) / 1000; + eta_s = (unsigned int)(elapsed_time + 500) / 1000; } else { eta_s = 0; } @@ -642,10 +640,17 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) /* compute current average values */ timediff = get_update_timediff(0);

- if(timediff < UPDATE_SPEED_MS) { + if(timediff < 0) { + /* we lost a chunk of time due to a clock change, including the amount + * xfered during that period in the next update would skew the rate */ + xfered_last = xfered; + return; + } else if(timediff < UPDATE_SPEED_MS) { /* return if the calling interval was too short */ return; } + + elapsed_time += timediff; rate = (double)(xfered - xfered_last) / (timediff / 1000.0); /* average rate to reduce jumpiness */ rate = (rate + 2 * rate_last) / 3; -- 1.8.4.2

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 --- 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 #include #include -#include #include /* off_t */ #include #include @@ -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 +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(¤t_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(¤t_time); + timediff = diff_timespec_ms(¤t_time, &initial_time); if(timediff > 0) { rate = (double)xfered / (timediff / 1000.0); /* round elapsed time (in ms) to the nearest second */ -- 2.0.1