* [PATCH] makedumpfile: Do not print ETA value if current progress is 0 @ 2018-04-09 9:40 Petr Tesarik 2018-04-10 7:09 ` lijiang 0 siblings, 1 reply; 10+ messages in thread From: Petr Tesarik @ 2018-04-09 9:40 UTC (permalink / raw) To: Masaki Tachibana, Takuya Nakayama, Daisuke Nishimura, Lianbo Jiang, kexec-ml Essentially, the estimated remaining time is calculated as: elapsed * (100 - progress) / progress However, print_progress() is also called when progress is 0. The result of a floating point division by zero is either NaN (if elapsed is zero), or infinity (if the system clock happens to cross a second's boundary since reading the start timestamp). The C standard defines only conversion of floating point values within the range of the destination integer variable. This means that conversion of NaN or infinity to an integer is undefined behaviour. Yes, it happens to produce INT_MIN with GCC on major platforms... This bug has gone unnoticed, because the very first call to print_progress() does not specify a start timestamp (so it cannot trigger the bug), and all subsequent calls are rate-limited to one per second. As a result, the bug is triggered very rarely. Before commit e5f96e79d69a1d295f19130da00ec6514d28a8ae, the bug also caused a buffer overflow. The buffer overflow is mitigated thanks to using snprintf() instead of sprintf(), but the program may still invoke undefined behaviour. Note that all other changes in the above-mentioned commit were ineffective. They merely reduced the precision of the calculation: Why would you add delta.tv_usec as a fraction if the fractional part is immediately truncated by a converstion to int64_t? Additionally, when the original bug is hit, the output is still incorrect, e.g. on my system I get: Copying data : [ 0.0 %] / eta: -9223372036854775808s For that reason, let me revert the changes from commit e5f96e79d69a1d295f19130da00ec6514d28a8ae and fix the bug properly, i.e. do not calculate ETA if progress is 0. Last but not least, part of the issue was probably caused by the wrong assumption that integers < 100 can be interpreted with max 3 ASCII characters, but that's not true for signed integers. To make eta_to_human_short() a bit safer, use an unsigned integer type. Signed-off-by: Petr Tesarik <ptesarik@suse.com> --- print_info.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/print_info.c b/print_info.c index 09e215a..ed701ab 100644 --- a/print_info.c +++ b/print_info.c @@ -16,8 +16,6 @@ #include "print_info.h" #include <time.h> #include <string.h> -#include <stdint.h> -#include <inttypes.h> #define PROGRESS_MAXLEN "50" @@ -354,21 +352,18 @@ static void calc_delta(struct timeval *tv_start, struct timeval *delta) } /* produce less than 12 bytes on msg */ -static int eta_to_human_short (int64_t secs, char* msg, int maxsize) +static int eta_to_human_short (unsigned secs, char* msg) { strcpy(msg, "eta: "); msg += strlen("eta: "); if (secs < 100) - snprintf(msg, maxsize, "%"PRId64"s", secs); + sprintf(msg, "%us", secs); else if (secs < 100 * 60) - snprintf(msg, maxsize, "%"PRId64"m""%"PRId64"s", - secs / 60, secs % 60); + sprintf(msg, "%um%us", secs / 60, secs % 60); else if (secs < 48 * 3600) - snprintf(msg, maxsize, "%"PRId64"h""%"PRId64"m", - secs / 3600, (secs / 60) % 60); + sprintf(msg, "%uh%um", secs / 3600, (secs / 60) % 60); else if (secs < 100 * 86400) - snprintf(msg, maxsize, "%"PRId64"d""%"PRId64"h", - secs / 86400, (secs / 3600) % 24); + sprintf(msg, "%ud%uh", secs / 86400, (secs / 3600) % 24); else sprintf(msg, ">2day"); return 0; @@ -384,8 +379,8 @@ print_progress(const char *msg, unsigned long current, unsigned long end, struct static unsigned int lapse = 0; static const char *spinner = "/|\\-"; struct timeval delta; - int64_t eta; - char eta_msg[32] = " "; + double eta; + char eta_msg[16] = " "; if (current < end) { tm = time(NULL); @@ -396,11 +391,11 @@ print_progress(const char *msg, unsigned long current, unsigned long end, struct } else progress = 100; - if (start != NULL) { + if (start != NULL && current != 0) { calc_delta(start, &delta); eta = delta.tv_sec + delta.tv_usec / 1e6; eta = (100 - progress) * eta / progress; - eta_to_human_short(eta, eta_msg, sizeof(eta_msg)); + eta_to_human_short(eta, eta_msg); } if (flag_ignore_r_char) { PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s\n", -- 2.13.6 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-04-09 9:40 [PATCH] makedumpfile: Do not print ETA value if current progress is 0 Petr Tesarik @ 2018-04-10 7:09 ` lijiang 2018-04-10 7:47 ` Petr Tesarik 0 siblings, 1 reply; 10+ messages in thread From: lijiang @ 2018-04-10 7:09 UTC (permalink / raw) To: Petr Tesarik, Masaki Tachibana, Takuya Nakayama, Daisuke Nishimura, kexec-ml 在 2018年04月09日 17:40, Petr Tesarik 写道: > Essentially, the estimated remaining time is calculated as: > > elapsed * (100 - progress) / progress > > However, print_progress() is also called when progress is 0. The > result of a floating point division by zero is either NaN (if > elapsed is zero), or infinity (if the system clock happens to cross > a second's boundary since reading the start timestamp). > > The C standard defines only conversion of floating point values > within the range of the destination integer variable. This means > that conversion of NaN or infinity to an integer is undefined > behaviour. Yes, it happens to produce INT_MIN with GCC on major > platforms... > > This bug has gone unnoticed, because the very first call to > print_progress() does not specify a start timestamp (so it cannot > trigger the bug), and all subsequent calls are rate-limited to one > per second. As a result, the bug is triggered very rarely. > > Before commit e5f96e79d69a1d295f19130da00ec6514d28a8ae, the bug also > caused a buffer overflow. The buffer overflow is mitigated thanks to > using snprintf() instead of sprintf(), but the program may still > invoke undefined behaviour. > > Note that all other changes in the above-mentioned commit were > ineffective. They merely reduced the precision of the calculation: > Why would you add delta.tv_usec as a fraction if the fractional part > is immediately truncated by a converstion to int64_t? > > Additionally, when the original bug is hit, the output is still > incorrect, e.g. on my system I get: > > Copying data : [ 0.0 %] / eta: -9223372036854775808s > > For that reason, let me revert the changes from commit > e5f96e79d69a1d295f19130da00ec6514d28a8ae and fix the bug properly, > i.e. do not calculate ETA if progress is 0. > > Last but not least, part of the issue was probably caused by the > wrong assumption that integers < 100 can be interpreted with max 3 > ASCII characters, but that's not true for signed integers. To make > eta_to_human_short() a bit safer, use an unsigned integer type. > > Signed-off-by: Petr Tesarik <ptesarik@suse.com> Hi, Petr It is good that checks the value of "current". The previous patch resolved system crash in exceptional case. The calculation result of "calc_delta" may be very large number or even a negative number when making the time jump very great. In this case, it is not enough for "unsigned integer type" and double. The struct timeval has two 64bit variables in x86 64. Thanks. Lianbo > --- > print_info.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/print_info.c b/print_info.c > index 09e215a..ed701ab 100644 > --- a/print_info.c > +++ b/print_info.c > @@ -16,8 +16,6 @@ > #include "print_info.h" > #include <time.h> > #include <string.h> > -#include <stdint.h> > -#include <inttypes.h> > > #define PROGRESS_MAXLEN "50" > > @@ -354,21 +352,18 @@ static void calc_delta(struct timeval *tv_start, struct timeval *delta) > } > > /* produce less than 12 bytes on msg */ > -static int eta_to_human_short (int64_t secs, char* msg, int maxsize) > +static int eta_to_human_short (unsigned secs, char* msg) > { > strcpy(msg, "eta: "); > msg += strlen("eta: "); > if (secs < 100) > - snprintf(msg, maxsize, "%"PRId64"s", secs); > + sprintf(msg, "%us", secs); > else if (secs < 100 * 60) > - snprintf(msg, maxsize, "%"PRId64"m""%"PRId64"s", > - secs / 60, secs % 60); > + sprintf(msg, "%um%us", secs / 60, secs % 60); > else if (secs < 48 * 3600) > - snprintf(msg, maxsize, "%"PRId64"h""%"PRId64"m", > - secs / 3600, (secs / 60) % 60); > + sprintf(msg, "%uh%um", secs / 3600, (secs / 60) % 60); > else if (secs < 100 * 86400) > - snprintf(msg, maxsize, "%"PRId64"d""%"PRId64"h", > - secs / 86400, (secs / 3600) % 24); > + sprintf(msg, "%ud%uh", secs / 86400, (secs / 3600) % 24); > else > sprintf(msg, ">2day"); > return 0; > @@ -384,8 +379,8 @@ print_progress(const char *msg, unsigned long current, unsigned long end, struct > static unsigned int lapse = 0; > static const char *spinner = "/|\\-"; > struct timeval delta; > - int64_t eta; > - char eta_msg[32] = " "; > + double eta; > + char eta_msg[16] = " "; > > if (current < end) { > tm = time(NULL); > @@ -396,11 +391,11 @@ print_progress(const char *msg, unsigned long current, unsigned long end, struct > } else > progress = 100; > > - if (start != NULL) { > + if (start != NULL && current != 0) { > calc_delta(start, &delta); > eta = delta.tv_sec + delta.tv_usec / 1e6; > eta = (100 - progress) * eta / progress; > - eta_to_human_short(eta, eta_msg, sizeof(eta_msg)); > + eta_to_human_short(eta, eta_msg); > } > if (flag_ignore_r_char) { > PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s\n", > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-04-10 7:09 ` lijiang @ 2018-04-10 7:47 ` Petr Tesarik 2018-04-10 8:28 ` Petr Tesarik 2018-06-20 3:07 ` [PATCH] makedumpfile: Do not print ETA value if current progress is 0 lijiang 0 siblings, 2 replies; 10+ messages in thread From: Petr Tesarik @ 2018-04-10 7:47 UTC (permalink / raw) To: lijiang; +Cc: Masaki Tachibana, Daisuke Nishimura, Takuya Nakayama, kexec-ml On Tue, 10 Apr 2018 15:09:37 +0800 lijiang <lijiang@redhat.com> wrote: > 在 2018年04月09日 17:40, Petr Tesarik 写道: >[...] > > Last but not least, part of the issue was probably caused by the > > wrong assumption that integers < 100 can be interpreted with max 3 > > ASCII characters, but that's not true for signed integers. To make > > eta_to_human_short() a bit safer, use an unsigned integer type. > > > Signed-off-by: Petr Tesarik <ptesarik@suse.com> > Hi, Petr > It is good that checks the value of "current". > The previous patch resolved system crash in exceptional case. The calculation > result of "calc_delta" may be very large number Hmmm. The resulting ETA is in seconds. To overflow a 32-bit integer, the elapsed time would have to be more than 2^32 seconds, or approx. 136 years. I think it is reasonable to assume that makedumpfile will never run for more than a century... > or even a negative number when making the time jump very great. Yes, the current code will not produce sensible results if the system clock moves backwards. With my patch it will most likely say: ">2day". But it will not work any better if you use a 64-bit integer. Instead, we should be using clock_gettime(CLOCK_MONOTONIC, ...). That's a separate issue that I can solve in another patch. > In this case, it is not enough for "unsigned > integer type" and double. The struct timeval has two 64bit variables in x86 64. First, of the second field (tv_usec), only 20 bits may actually be used, because it can never be greater than one million. Second, to make 100% sure that the target value fits into the eta variable, you would have to consider the worst case: end = ULONG_MAX current = 1 progress = 100.0 / ULONG_MAX delta = { ULONG_MAX, 999999 } eta = ULONG_MAX + 999999/1e6 eta = (100 - progress) * eta / progress Theoretically, this gives: eta = 340345831618257409870852130465035835837 Even a 128-bit integer is too small to hold this maximum theoretical value (129 bits would be needed). Let's stay reasonable. Any value which represents more than a few (dozen) hours is not usable in practice. But hey, to make sure we cannot hit undefined behaviour, why not pass a double to eta_to_human_short()... Going to send an updated patch. Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-04-10 7:47 ` Petr Tesarik @ 2018-04-10 8:28 ` Petr Tesarik 2018-04-10 11:39 ` [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar Petr Tesarik 2018-06-20 3:07 ` [PATCH] makedumpfile: Do not print ETA value if current progress is 0 lijiang 1 sibling, 1 reply; 10+ messages in thread From: Petr Tesarik @ 2018-04-10 8:28 UTC (permalink / raw) To: lijiang; +Cc: Masaki Tachibana, Takuya Nakayama, kexec-ml On Tue, 10 Apr 2018 09:47:07 +0200 Petr Tesarik <ptesarik@suse.cz> wrote: >[...] > Let's stay reasonable. Any value which represents more than a few > (dozen) hours is not usable in practice. But hey, to make sure we > cannot hit undefined behaviour, why not pass a double to > eta_to_human_short()... I gave it a second thought. Using math library functions just to print a progress bar is overkill. In fact, using the FPU is already overkill. Instead, I'm going to convert the calculations to pure integer arithmetic. Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar 2018-04-10 8:28 ` Petr Tesarik @ 2018-04-10 11:39 ` Petr Tesarik 2018-05-18 8:22 ` Masaki Tachibana 0 siblings, 1 reply; 10+ messages in thread From: Petr Tesarik @ 2018-04-10 11:39 UTC (permalink / raw) To: lijiang; +Cc: Masaki Tachibana, Takuya Nakayama, kexec-ml Essentially, the estimated remaining time is calculated as: elapsed * (100 - progress) / progress Since the calculation is done with floating point numbers, it had masked a division by zero (if progress is 0), producing a NaN or infinity. The following conversion to int produces INT_MIN with GCC on major platforms, which originally overflowed the eta buffer. This bug was fixed by commit e5f96e79d69a1d295f19130da00ec6514d28a8ae, but conversion of NaN and infinity is undefined behaviour in ISO C, plus the corresponding output is still wrong, e.g.: Copying data : [ 0.0 %] / eta: -9223372036854775808s Most importantly, using the FPU for a progress bar is overkill. Since the progress percentage is reported with one decimal digit following the decimal point, it can be stored as an integer tenths of a percent. Second, the estimated time can be calculated in milliseconds. Up to 49 days can be represented this way even on 32-bit platforms. Note that delta.tv_usec can be ignored in the subtraction, because the resulting eta is printed as seconds, so elapsed microseconds are irrelevant. Last but not least, the original buffer overflow was probably caused by the wrong assumption that integers < 100 can be interpreted with less than 3 ASCII characters, but that's not true for signed integers. To make eta_to_human_short() a bit safer, use an unsigned integer type. Signed-off-by: Petr Tesarik <ptesarik@suse.com> --- print_info.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/print_info.c b/print_info.c index 09e215a..6bfcd11 100644 --- a/print_info.c +++ b/print_info.c @@ -16,8 +16,6 @@ #include "print_info.h" #include <time.h> #include <string.h> -#include <stdint.h> -#include <inttypes.h> #define PROGRESS_MAXLEN "50" @@ -354,21 +352,18 @@ static void calc_delta(struct timeval *tv_start, struct timeval *delta) } /* produce less than 12 bytes on msg */ -static int eta_to_human_short (int64_t secs, char* msg, int maxsize) +static int eta_to_human_short (unsigned long secs, char* msg) { strcpy(msg, "eta: "); msg += strlen("eta: "); if (secs < 100) - snprintf(msg, maxsize, "%"PRId64"s", secs); + sprintf(msg, "%lus", secs); else if (secs < 100 * 60) - snprintf(msg, maxsize, "%"PRId64"m""%"PRId64"s", - secs / 60, secs % 60); + sprintf(msg, "%lum%lus", secs / 60, secs % 60); else if (secs < 48 * 3600) - snprintf(msg, maxsize, "%"PRId64"h""%"PRId64"m", - secs / 3600, (secs / 60) % 60); + sprintf(msg, "%luh%lum", secs / 3600, (secs / 60) % 60); else if (secs < 100 * 86400) - snprintf(msg, maxsize, "%"PRId64"d""%"PRId64"h", - secs / 86400, (secs / 3600) % 24); + sprintf(msg, "%lud%luh", secs / 86400, (secs / 3600) % 24); else sprintf(msg, ">2day"); return 0; @@ -378,37 +373,39 @@ static int eta_to_human_short (int64_t secs, char* msg, int maxsize) void print_progress(const char *msg, unsigned long current, unsigned long end, struct timeval *start) { - float progress; + unsigned progress; /* in promilles (tenths of a percent) */ time_t tm; static time_t last_time = 0; static unsigned int lapse = 0; static const char *spinner = "/|\\-"; struct timeval delta; - int64_t eta; - char eta_msg[32] = " "; + unsigned long eta; + char eta_msg[16] = " "; if (current < end) { tm = time(NULL); if (tm - last_time < 1) return; last_time = tm; - progress = (float)current * 100 / end; + progress = current * 1000 / end; } else - progress = 100; + progress = 1000; - if (start != NULL) { + if (start != NULL && progress != 0) { calc_delta(start, &delta); - eta = delta.tv_sec + delta.tv_usec / 1e6; - eta = (100 - progress) * eta / progress; - eta_to_human_short(eta, eta_msg, sizeof(eta_msg)); + eta = 1000 * delta.tv_sec + delta.tv_usec / 1000; + eta = eta / progress - delta.tv_sec; + eta_to_human_short(eta, eta_msg); } if (flag_ignore_r_char) { - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s\n", - msg, progress, spinner[lapse % 4], eta_msg); + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s\n", + msg, progress / 10, progress % 10, + spinner[lapse % 4], eta_msg); } else { PROGRESS_MSG("\r"); - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s", - msg, progress, spinner[lapse % 4], eta_msg); + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s", + msg, progress / 10, progress % 10, + spinner[lapse % 4], eta_msg); } lapse++; } -- 2.13.6 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar 2018-04-10 11:39 ` [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar Petr Tesarik @ 2018-05-18 8:22 ` Masaki Tachibana 2018-05-18 12:46 ` Petr Tesarik 0 siblings, 1 reply; 10+ messages in thread From: Masaki Tachibana @ 2018-05-18 8:22 UTC (permalink / raw) To: Petr Tesarik, lijiang; +Cc: kexec-ml, Keiichi Nakamura Hi Petr and Lianbo, Sorry for the late reply. I will merge the patch into V1.6.4. Thanks Tachibana > -----Original Message----- > From: Petr Tesarik [mailto:ptesarik@suse.cz] > Sent: Tuesday, April 10, 2018 8:39 PM > To: lijiang <lijiang@redhat.com> > Cc: Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>; Nakayama Takuya() <tak-nakayama@tg.jp.nec.com>; > kexec-ml <kexec@lists.infradead.org> > Subject: [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar > > Essentially, the estimated remaining time is calculated as: > > elapsed * (100 - progress) / progress > > Since the calculation is done with floating point numbers, it had > masked a division by zero (if progress is 0), producing a NaN or > infinity. The following conversion to int produces INT_MIN with GCC > on major platforms, which originally overflowed the eta buffer. This > bug was fixed by commit e5f96e79d69a1d295f19130da00ec6514d28a8ae, > but conversion of NaN and infinity is undefined behaviour in ISO C, > plus the corresponding output is still wrong, e.g.: > > Copying data : [ 0.0 %] / eta: -9223372036854775808s > > Most importantly, using the FPU for a progress bar is overkill. > Since the progress percentage is reported with one decimal digit > following the decimal point, it can be stored as an integer tenths > of a percent. > > Second, the estimated time can be calculated in milliseconds. Up to > 49 days can be represented this way even on 32-bit platforms. Note > that delta.tv_usec can be ignored in the subtraction, because the > resulting eta is printed as seconds, so elapsed microseconds are > irrelevant. > > Last but not least, the original buffer overflow was probably caused > by the wrong assumption that integers < 100 can be interpreted with > less than 3 ASCII characters, but that's not true for signed > integers. To make eta_to_human_short() a bit safer, use an unsigned > integer type. > > Signed-off-by: Petr Tesarik <ptesarik@suse.com> > --- > print_info.c | 43 ++++++++++++++++++++----------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/print_info.c b/print_info.c > index 09e215a..6bfcd11 100644 > --- a/print_info.c > +++ b/print_info.c > @@ -16,8 +16,6 @@ > #include "print_info.h" > #include <time.h> > #include <string.h> > -#include <stdint.h> > -#include <inttypes.h> > > #define PROGRESS_MAXLEN "50" > > @@ -354,21 +352,18 @@ static void calc_delta(struct timeval *tv_start, struct timeval *delta) > } > > /* produce less than 12 bytes on msg */ > -static int eta_to_human_short (int64_t secs, char* msg, int maxsize) > +static int eta_to_human_short (unsigned long secs, char* msg) > { > strcpy(msg, "eta: "); > msg += strlen("eta: "); > if (secs < 100) > - snprintf(msg, maxsize, "%"PRId64"s", secs); > + sprintf(msg, "%lus", secs); > else if (secs < 100 * 60) > - snprintf(msg, maxsize, "%"PRId64"m""%"PRId64"s", > - secs / 60, secs % 60); > + sprintf(msg, "%lum%lus", secs / 60, secs % 60); > else if (secs < 48 * 3600) > - snprintf(msg, maxsize, "%"PRId64"h""%"PRId64"m", > - secs / 3600, (secs / 60) % 60); > + sprintf(msg, "%luh%lum", secs / 3600, (secs / 60) % 60); > else if (secs < 100 * 86400) > - snprintf(msg, maxsize, "%"PRId64"d""%"PRId64"h", > - secs / 86400, (secs / 3600) % 24); > + sprintf(msg, "%lud%luh", secs / 86400, (secs / 3600) % 24); > else > sprintf(msg, ">2day"); > return 0; > @@ -378,37 +373,39 @@ static int eta_to_human_short (int64_t secs, char* msg, int maxsize) > void > print_progress(const char *msg, unsigned long current, unsigned long end, struct timeval *start) > { > - float progress; > + unsigned progress; /* in promilles (tenths of a percent) */ > time_t tm; > static time_t last_time = 0; > static unsigned int lapse = 0; > static const char *spinner = "/|\\-"; > struct timeval delta; > - int64_t eta; > - char eta_msg[32] = " "; > + unsigned long eta; > + char eta_msg[16] = " "; > > if (current < end) { > tm = time(NULL); > if (tm - last_time < 1) > return; > last_time = tm; > - progress = (float)current * 100 / end; > + progress = current * 1000 / end; > } else > - progress = 100; > + progress = 1000; > > - if (start != NULL) { > + if (start != NULL && progress != 0) { > calc_delta(start, &delta); > - eta = delta.tv_sec + delta.tv_usec / 1e6; > - eta = (100 - progress) * eta / progress; > - eta_to_human_short(eta, eta_msg, sizeof(eta_msg)); > + eta = 1000 * delta.tv_sec + delta.tv_usec / 1000; > + eta = eta / progress - delta.tv_sec; > + eta_to_human_short(eta, eta_msg); > } > if (flag_ignore_r_char) { > - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s\n", > - msg, progress, spinner[lapse % 4], eta_msg); > + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s\n", > + msg, progress / 10, progress % 10, > + spinner[lapse % 4], eta_msg); > } else { > PROGRESS_MSG("\r"); > - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s", > - msg, progress, spinner[lapse % 4], eta_msg); > + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s", > + msg, progress / 10, progress % 10, > + spinner[lapse % 4], eta_msg); > } > lapse++; > } > -- > 2.13.6 > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar 2018-05-18 8:22 ` Masaki Tachibana @ 2018-05-18 12:46 ` Petr Tesarik 0 siblings, 0 replies; 10+ messages in thread From: Petr Tesarik @ 2018-05-18 12:46 UTC (permalink / raw) To: Masaki Tachibana; +Cc: kexec-ml, lijiang, Keiichi Nakamura On Fri, 18 May 2018 08:22:10 +0000 Masaki Tachibana <mas-tachibana@vf.jp.nec.com> wrote: > Hi Petr and Lianbo, Hi Masaki-san, > Sorry for the late reply. No problem. I know how to wait. ;-) > I will merge the patch into V1.6.4. Thank you very much! > > Thanks > Tachibana > > > > -----Original Message----- > > From: Petr Tesarik [mailto:ptesarik@suse.cz] > > Sent: Tuesday, April 10, 2018 8:39 PM > > To: lijiang <lijiang@redhat.com> > > Cc: Tachibana Masaki() <mas-tachibana@vf.jp.nec.com>; Nakayama Takuya() <tak-nakayama@tg.jp.nec.com>; > > kexec-ml <kexec@lists.infradead.org> > > Subject: [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar > > > > Essentially, the estimated remaining time is calculated as: > > > > elapsed * (100 - progress) / progress > > > > Since the calculation is done with floating point numbers, it had > > masked a division by zero (if progress is 0), producing a NaN or > > infinity. The following conversion to int produces INT_MIN with GCC > > on major platforms, which originally overflowed the eta buffer. This > > bug was fixed by commit e5f96e79d69a1d295f19130da00ec6514d28a8ae, > > but conversion of NaN and infinity is undefined behaviour in ISO C, > > plus the corresponding output is still wrong, e.g.: > > > > Copying data : [ 0.0 %] / eta: -9223372036854775808s > > > > Most importantly, using the FPU for a progress bar is overkill. > > Since the progress percentage is reported with one decimal digit > > following the decimal point, it can be stored as an integer tenths > > of a percent. > > > > Second, the estimated time can be calculated in milliseconds. Up to > > 49 days can be represented this way even on 32-bit platforms. Note > > that delta.tv_usec can be ignored in the subtraction, because the > > resulting eta is printed as seconds, so elapsed microseconds are > > irrelevant. > > > > Last but not least, the original buffer overflow was probably caused > > by the wrong assumption that integers < 100 can be interpreted with > > less than 3 ASCII characters, but that's not true for signed > > integers. To make eta_to_human_short() a bit safer, use an unsigned > > integer type. > > > > Signed-off-by: Petr Tesarik <ptesarik@suse.com> > > --- > > print_info.c | 43 ++++++++++++++++++++----------------------- > > 1 file changed, 20 insertions(+), 23 deletions(-) > > > > diff --git a/print_info.c b/print_info.c > > index 09e215a..6bfcd11 100644 > > --- a/print_info.c > > +++ b/print_info.c > > @@ -16,8 +16,6 @@ > > #include "print_info.h" > > #include <time.h> > > #include <string.h> > > -#include <stdint.h> > > -#include <inttypes.h> > > > > #define PROGRESS_MAXLEN "50" > > > > @@ -354,21 +352,18 @@ static void calc_delta(struct timeval *tv_start, struct timeval *delta) > > } > > > > /* produce less than 12 bytes on msg */ > > -static int eta_to_human_short (int64_t secs, char* msg, int maxsize) > > +static int eta_to_human_short (unsigned long secs, char* msg) > > { > > strcpy(msg, "eta: "); > > msg += strlen("eta: "); > > if (secs < 100) > > - snprintf(msg, maxsize, "%"PRId64"s", secs); > > + sprintf(msg, "%lus", secs); > > else if (secs < 100 * 60) > > - snprintf(msg, maxsize, "%"PRId64"m""%"PRId64"s", > > - secs / 60, secs % 60); > > + sprintf(msg, "%lum%lus", secs / 60, secs % 60); > > else if (secs < 48 * 3600) > > - snprintf(msg, maxsize, "%"PRId64"h""%"PRId64"m", > > - secs / 3600, (secs / 60) % 60); > > + sprintf(msg, "%luh%lum", secs / 3600, (secs / 60) % 60); > > else if (secs < 100 * 86400) > > - snprintf(msg, maxsize, "%"PRId64"d""%"PRId64"h", > > - secs / 86400, (secs / 3600) % 24); > > + sprintf(msg, "%lud%luh", secs / 86400, (secs / 3600) % 24); > > else > > sprintf(msg, ">2day"); > > return 0; > > @@ -378,37 +373,39 @@ static int eta_to_human_short (int64_t secs, char* msg, int maxsize) > > void > > print_progress(const char *msg, unsigned long current, unsigned long end, struct timeval *start) > > { > > - float progress; > > + unsigned progress; /* in promilles (tenths of a percent) */ > > time_t tm; > > static time_t last_time = 0; > > static unsigned int lapse = 0; > > static const char *spinner = "/|\\-"; > > struct timeval delta; > > - int64_t eta; > > - char eta_msg[32] = " "; > > + unsigned long eta; > > + char eta_msg[16] = " "; > > > > if (current < end) { > > tm = time(NULL); > > if (tm - last_time < 1) > > return; > > last_time = tm; > > - progress = (float)current * 100 / end; > > + progress = current * 1000 / end; > > } else > > - progress = 100; > > + progress = 1000; > > > > - if (start != NULL) { > > + if (start != NULL && progress != 0) { > > calc_delta(start, &delta); > > - eta = delta.tv_sec + delta.tv_usec / 1e6; > > - eta = (100 - progress) * eta / progress; > > - eta_to_human_short(eta, eta_msg, sizeof(eta_msg)); > > + eta = 1000 * delta.tv_sec + delta.tv_usec / 1000; > > + eta = eta / progress - delta.tv_sec; > > + eta_to_human_short(eta, eta_msg); > > } > > if (flag_ignore_r_char) { > > - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s\n", > > - msg, progress, spinner[lapse % 4], eta_msg); > > + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s\n", > > + msg, progress / 10, progress % 10, > > + spinner[lapse % 4], eta_msg); > > } else { > > PROGRESS_MSG("\r"); > > - PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%5.1f %%] %c %16s", > > - msg, progress, spinner[lapse % 4], eta_msg); > > + PROGRESS_MSG("%-" PROGRESS_MAXLEN "s: [%3u.%u %%] %c %16s", > > + msg, progress / 10, progress % 10, > > + spinner[lapse % 4], eta_msg); > > } > > lapse++; > > } > > -- > > 2.13.6 > > > > > > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-04-10 7:47 ` Petr Tesarik 2018-04-10 8:28 ` Petr Tesarik @ 2018-06-20 3:07 ` lijiang 2018-06-20 7:17 ` Petr Tesarik 1 sibling, 1 reply; 10+ messages in thread From: lijiang @ 2018-06-20 3:07 UTC (permalink / raw) To: Petr Tesarik Cc: Masaki Tachibana, Daisuke Nishimura, Dave Young, Takuya Nakayama, kexec-ml 在 2018年04月10日 15:47, Petr Tesarik 写道: > On Tue, 10 Apr 2018 15:09:37 +0800 > lijiang <lijiang@redhat.com> wrote: > >> 在 2018年04月09日 17:40, Petr Tesarik 写道: >> [...] >>> Last but not least, part of the issue was probably caused by the >>> wrong assumption that integers < 100 can be interpreted with max 3 >>> ASCII characters, but that's not true for signed integers. To make >>> eta_to_human_short() a bit safer, use an unsigned integer type. >>>> Signed-off-by: Petr Tesarik <ptesarik@suse.com> >> Hi, Petr >> It is good that checks the value of "current". >> The previous patch resolved system crash in exceptional case. The calculation >> result of "calc_delta" may be very large number > > Hmmm. The resulting ETA is in seconds. To overflow a 32-bit integer, > the elapsed time would have to be more than 2^32 seconds, or approx. > 136 years. I think it is reasonable to assume that makedumpfile will > never run for more than a century... > >> or even a negative number when making the time jump very great. > > Yes, the current code will not produce sensible results if the system > clock moves backwards. With my patch it will most likely say: ">2day". > But it will not work any better if you use a 64-bit integer. > > Instead, we should be using clock_gettime(CLOCK_MONOTONIC, ...). > That's a separate issue that I can solve in another patch. Hi, Petr Previously mentioned that you would solve this issue, would you have any plans about this issue? Thanks. Lianbo >> In this case, it is not enough for "unsigned >> integer type" and double. The struct timeval has two 64bit variables in x86 64. > > First, of the second field (tv_usec), only 20 bits may actually be > used, because it can never be greater than one million. > > Second, to make 100% sure that the target value fits into the eta > variable, you would have to consider the worst case: > > end = ULONG_MAX > current = 1 > progress = 100.0 / ULONG_MAX > delta = { ULONG_MAX, 999999 } > eta = ULONG_MAX + 999999/1e6 > eta = (100 - progress) * eta / progress > > Theoretically, this gives: > > eta = 340345831618257409870852130465035835837 > > Even a 128-bit integer is too small to hold this maximum theoretical > value (129 bits would be needed). > > Let's stay reasonable. Any value which represents more than a few > (dozen) hours is not usable in practice. But hey, to make sure we > cannot hit undefined behaviour, why not pass a double to > eta_to_human_short()... > > Going to send an updated patch. > > Petr T > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-06-20 3:07 ` [PATCH] makedumpfile: Do not print ETA value if current progress is 0 lijiang @ 2018-06-20 7:17 ` Petr Tesarik 2018-06-20 11:24 ` lijiang 0 siblings, 1 reply; 10+ messages in thread From: Petr Tesarik @ 2018-06-20 7:17 UTC (permalink / raw) To: lijiang Cc: Masaki Tachibana, Daisuke Nishimura, Dave Young, Takuya Nakayama, kexec-ml Hi Lianbo, On Wed, 20 Jun 2018 11:07:46 +0800 lijiang <lijiang@redhat.com> wrote: > 在 2018年04月10日 15:47, Petr Tesarik 写道: >[...] > > Yes, the current code will not produce sensible results if the system > > clock moves backwards. With my patch it will most likely say: ">2day". > > But it will not work any better if you use a 64-bit integer. > > > > Instead, we should be using clock_gettime(CLOCK_MONOTONIC, ...). > > That's a separate issue that I can solve in another patch. > Hi, Petr > Previously mentioned that you would solve this issue, would you have any plans > about this issue? Thank you for the reminder. Yes, let me do it now! Petr T _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] makedumpfile: Do not print ETA value if current progress is 0 2018-06-20 7:17 ` Petr Tesarik @ 2018-06-20 11:24 ` lijiang 0 siblings, 0 replies; 10+ messages in thread From: lijiang @ 2018-06-20 11:24 UTC (permalink / raw) To: Petr Tesarik Cc: Masaki Tachibana, Daisuke Nishimura, Dave Young, Takuya Nakayama, kexec-ml 在 2018年06月20日 15:17, Petr Tesarik 写道: > Hi Lianbo, > > On Wed, 20 Jun 2018 11:07:46 +0800 > lijiang <lijiang@redhat.com> wrote: > >> 在 2018年04月10日 15:47, Petr Tesarik 写道: >> [...] >>> Yes, the current code will not produce sensible results if the system >>> clock moves backwards. With my patch it will most likely say: ">2day". >>> But it will not work any better if you use a 64-bit integer. >>> >>> Instead, we should be using clock_gettime(CLOCK_MONOTONIC, ...). >>> That's a separate issue that I can solve in another patch. >> Hi, Petr >> Previously mentioned that you would solve this issue, would you have any plans >> about this issue? > > Thank you for the reminder. Yes, let me do it now! > > Petr T > Swift action! I have seen your patches, thank you. Lianbo _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-06-20 11:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-09 9:40 [PATCH] makedumpfile: Do not print ETA value if current progress is 0 Petr Tesarik 2018-04-10 7:09 ` lijiang 2018-04-10 7:47 ` Petr Tesarik 2018-04-10 8:28 ` Petr Tesarik 2018-04-10 11:39 ` [PATCH v2] makedumpfile: Use integer arithmetics for the progress bar Petr Tesarik 2018-05-18 8:22 ` Masaki Tachibana 2018-05-18 12:46 ` Petr Tesarik 2018-06-20 3:07 ` [PATCH] makedumpfile: Do not print ETA value if current progress is 0 lijiang 2018-06-20 7:17 ` Petr Tesarik 2018-06-20 11:24 ` lijiang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.