| Message ID | 20240416123427.614899-3-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfsprogs random fixes found by Coverity scan | expand |
On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote: > In most of the uses of duration() takes time_t instead of int. > Convert the rest to use time_t and make duration() take time_t to > not truncate it to int. > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > --- > repair/progress.c | 4 ++-- > repair/progress.h | 2 +- > repair/xfs_repair.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/repair/progress.c b/repair/progress.c > index f6c4d988444e..c2af1387eb14 100644 > --- a/repair/progress.c > +++ b/repair/progress.c > @@ -273,7 +273,7 @@ progress_rpt_thread (void *p) > _("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"), > tmp->tm_hour, tmp->tm_min, tmp->tm_sec, > current_phase, percent, > - duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); > + duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); I'm not in love with how this expression mixes uint64_t and time_t, but I guess it's all the same now that we forced time_t to 64 bits. You might remove the pointless parentheses around elapsed. > } > > if (pthread_mutex_unlock(&msgp->mutex) != 0) { > @@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf) > } > > char * > -duration(int length, char *buf) > +duration(time_t length, char *buf) > { > int sum; > int weeks; > diff --git a/repair/progress.h b/repair/progress.h > index 2c1690db1b17..9575df164aa0 100644 > --- a/repair/progress.h > +++ b/repair/progress.h > @@ -38,7 +38,7 @@ extern void summary_report(void); > extern int set_progress_msg(int report, uint64_t total); > extern uint64_t print_final_rpt(void); > extern char *timestamp(int end, int phase, char *buf); > -extern char *duration(int val, char *buf); > +extern char *duration(time_t val, char *buf); > extern int do_parallel; > > #define PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b) > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > index ba9d28330d82..78a7205f0054 100644 > --- a/repair/xfs_repair.c > +++ b/repair/xfs_repair.c > @@ -377,7 +377,7 @@ process_args(int argc, char **argv) > do_prefetch = 0; > break; > case 't': > - report_interval = (int)strtol(optarg, NULL, 0); > + report_interval = (time_t)strtol(optarg, NULL, 0); report_interval is declared as an int and this patch doesn't change that. <confused> --D > break; > case 'e': > report_corrected = true; > -- > 2.42.0 > >
On 2024-04-16 09:12:30, Darrick J. Wong wrote: > On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote: > > In most of the uses of duration() takes time_t instead of int. > > Convert the rest to use time_t and make duration() take time_t to > > not truncate it to int. > > > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > > --- > > repair/progress.c | 4 ++-- > > repair/progress.h | 2 +- > > repair/xfs_repair.c | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/repair/progress.c b/repair/progress.c > > index f6c4d988444e..c2af1387eb14 100644 > > --- a/repair/progress.c > > +++ b/repair/progress.c > > @@ -273,7 +273,7 @@ progress_rpt_thread (void *p) > > _("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"), > > tmp->tm_hour, tmp->tm_min, tmp->tm_sec, > > current_phase, percent, > > - duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); > > + duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); > > I'm not in love with how this expression mixes uint64_t and time_t, but > I guess it's all the same now that we forced time_t to 64 bits. > > You might remove the pointless parentheses around elapsed. sure > > > } > > > > if (pthread_mutex_unlock(&msgp->mutex) != 0) { > > @@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf) > > } > > > > char * > > -duration(int length, char *buf) > > +duration(time_t length, char *buf) > > { > > int sum; > > int weeks; > > diff --git a/repair/progress.h b/repair/progress.h > > index 2c1690db1b17..9575df164aa0 100644 > > --- a/repair/progress.h > > +++ b/repair/progress.h > > @@ -38,7 +38,7 @@ extern void summary_report(void); > > extern int set_progress_msg(int report, uint64_t total); > > extern uint64_t print_final_rpt(void); > > extern char *timestamp(int end, int phase, char *buf); > > -extern char *duration(int val, char *buf); > > +extern char *duration(time_t val, char *buf); > > extern int do_parallel; > > > > #define PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b) > > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c > > index ba9d28330d82..78a7205f0054 100644 > > --- a/repair/xfs_repair.c > > +++ b/repair/xfs_repair.c > > @@ -377,7 +377,7 @@ process_args(int argc, char **argv) > > do_prefetch = 0; > > break; > > case 't': > > - report_interval = (int)strtol(optarg, NULL, 0); > > + report_interval = (time_t)strtol(optarg, NULL, 0); > > report_interval is declared as an int and this patch doesn't change > that. <confused> ops, yeah, missed that. Will change it to report_interval > > --D > > > break; > > case 'e': > > report_corrected = true; > > -- > > 2.42.0 > > > > >
On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote: > @@ -273,7 +273,7 @@ progress_rpt_thread (void *p) > _("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"), > tmp->tm_hour, tmp->tm_min, tmp->tm_sec, > current_phase, percent, > - duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); > + duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); What is the point of the time_t cast the gets applied to the final calculated expression? Even if time_t is wieder than what it was, it would only extent it after we've overlflow the original type. The whole thing also just is formatted and filled with useles braces to make it look really odd. Something like: current_phase, percent, duration((*msgp->total - sum) * elapsed / sum, msgbuf)); would be way more readable.
diff --git a/repair/progress.c b/repair/progress.c index f6c4d988444e..c2af1387eb14 100644 --- a/repair/progress.c +++ b/repair/progress.c @@ -273,7 +273,7 @@ progress_rpt_thread (void *p) _("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"), tmp->tm_hour, tmp->tm_min, tmp->tm_sec, current_phase, percent, - duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); + duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf)); } if (pthread_mutex_unlock(&msgp->mutex) != 0) { @@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf) } char * -duration(int length, char *buf) +duration(time_t length, char *buf) { int sum; int weeks; diff --git a/repair/progress.h b/repair/progress.h index 2c1690db1b17..9575df164aa0 100644 --- a/repair/progress.h +++ b/repair/progress.h @@ -38,7 +38,7 @@ extern void summary_report(void); extern int set_progress_msg(int report, uint64_t total); extern uint64_t print_final_rpt(void); extern char *timestamp(int end, int phase, char *buf); -extern char *duration(int val, char *buf); +extern char *duration(time_t val, char *buf); extern int do_parallel; #define PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b) diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c index ba9d28330d82..78a7205f0054 100644 --- a/repair/xfs_repair.c +++ b/repair/xfs_repair.c @@ -377,7 +377,7 @@ process_args(int argc, char **argv) do_prefetch = 0; break; case 't': - report_interval = (int)strtol(optarg, NULL, 0); + report_interval = (time_t)strtol(optarg, NULL, 0); break; case 'e': report_corrected = true;
In most of the uses of duration() takes time_t instead of int. Convert the rest to use time_t and make duration() take time_t to not truncate it to int. Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- repair/progress.c | 4 ++-- repair/progress.h | 2 +- repair/xfs_repair.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)