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.
Fixes FS#36983
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com --- 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;
On 7 December 2013 05:32, Andrew Gregory andrew.gregory.8@gmail.com 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 andrew.gregory.8@gmail.com
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 12/07/13 at 10:13am, Martin Panter wrote:
On 7 December 2013 05:32, Andrew Gregory andrew.gregory.8@gmail.com 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.
Dave also pointed this out to me on IRC. Unfortunately, it looks like it is indeed not available on OS X. If there is a suitable replacement function on OS X we could go the platform-specific route, otherwise it hardly seems worth it as we'd just end up having to do all of the same hacks in the OS X branch. It looks like OS X has clock_get_time, but I'm having trouble finding proper documentation for it to determine if it's suitable.
apg
On 07/12/13 11:36, Andrew Gregory wrote:
On 12/07/13 at 10:13am, Martin Panter wrote:
On 7 December 2013 05:32, Andrew Gregory andrew.gregory.8@gmail.com 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.
Dave also pointed this out to me on IRC. Unfortunately, it looks like it is indeed not available on OS X. If there is a suitable replacement function on OS X we could go the platform-specific route, otherwise it hardly seems worth it as we'd just end up having to do all of the same hacks in the OS X branch. It looks like OS X has clock_get_time, but I'm having trouble finding proper documentation for it to determine if it's suitable.
Darwin is a second class citizen - look at all the things in makepkg that require ELF binaries. As long as it builds and has basic functionality, that is enough.
Have OSX use the current system, uses clock_gettime for the rest (I believe the BSDs/Hurd all have it).
Allan
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.
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) +{ + 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 */
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);
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);
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);-- 2.0.1
gettimeofday is susceptible to backwards 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.
Fixes FS#36983
Signed-off-by: Andrew Gregory andrew.gregory.8@gmail.com ---
* return time in milliseconds rather than time* structs * prefer coarse timer, it's faster and still has millisecond precision * use int64_t instead of long
src/pacman/callback.c | 58 +++++++++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 27 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 9cde1de..e3af22f 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -21,8 +21,9 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/time.h> +#include <sys/time.h> /* gettimeofday */ #include <sys/types.h> /* off_t */ +#include <stdint.h> /* int64_t */ #include <time.h> #include <unistd.h> #include <wchar.h> @@ -47,6 +48,24 @@ static alpm_list_t *output = NULL; /* update speed for the fill_progress based functions */ #define UPDATE_SPEED_MS 200
+#if !defined(CLOCK_MONOTONIC_COARSE) && defined(CLOCK_MONOTONIC) +#define CLOCK_MONOTONIC_COARSE CLOCK_MONOTONIC +#endif + +static int64_t get_time_ms(void) +{ +#if defined(_POSIX_TIMERS) && (_POSIX_TIMERS > 0) && defined(CLOCK_MONOTONIC_COARSE) + struct timespec ts = {0, 0}; + clock_gettime(CLOCK_MONOTONIC_COARSE, &ts); + return (ts.tv_sec * 1000) + (ts.tv_nsec / 1000000); +#else + /* darwin doesn't support clock_gettime, fallback to gettimeofday */ + struct timeval tv = {0, 0}; + gettimeofday(&tv, NULL); + return (tv.tv_sec * 1000) + (tv.tv_usec / 1000); +#endif +} + /** * Silly little helper function, determines if the caller needs a visual update * since the last time this function was called. @@ -54,27 +73,20 @@ static alpm_list_t *output = NULL; * @param first_call 1 on first call for initialization purposes, 0 otherwise * @return number of milliseconds since last call */ -static long get_update_timediff(int first_call) +static int64_t get_update_timediff(int first_call) { - long retval = 0; - static struct timeval last_time = {0, 0}; + int64_t retval = 0; + static int64_t last_time = 0;
/* on first call, simply set the last time and return */ if(first_call) { - gettimeofday(&last_time, NULL); + last_time = get_time_ms(); } 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); + int64_t this_time = get_time_ms(); + retval = 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 +659,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 int64_t initial_time = 0; int infolen; int filenamelen; char *fname, *p; @@ -658,7 +670,6 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) int totaldownload = 0; off_t xfered, total; double rate = 0.0; - long timediff = 0; unsigned int eta_h = 0, eta_m = 0, eta_s = 0; double rate_human, xfered_human; const char *rate_label, *xfered_label; @@ -709,21 +720,14 @@ 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); + initial_time = get_time_ms(); 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); + int64_t timediff = get_time_ms() - initial_time; if(timediff > 0) { rate = (double)xfered / (timediff / 1000.0); /* round elapsed time (in ms) to the nearest second */ @@ -733,7 +737,7 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) } } else { /* compute current average values */ - timediff = get_update_timediff(0); + int64_t timediff = get_update_timediff(0);
if(timediff < UPDATE_SPEED_MS) { /* return if the calling interval was too short */
pacman-dev@lists.archlinux.org