Commit 3131977d authored by Jeff King's avatar Jeff King Committed by Junio C Hamano

progress: store throughput display in a strbuf

Coverity noticed that we strncpy() into a fixed-size buffer
without making sure that it actually ended up
NUL-terminated. This is unlikely to be a bug in practice,
since throughput strings rarely hit 32 characters, but it
would be nice to clean it up.

The most obvious way to do so is to add a NUL-terminator.
But instead, this patch switches the fixed-size buffer out
for a strbuf. At first glance this seems much less
efficient, until we realize that filling in the fixed-size
buffer is done by writing into a strbuf and copying the
result!

By writing straight to the buffer, we actually end up more
efficient:

  1. We avoid an extra copy of the bytes.

  2. Rather than malloc/free each time progress is shown, we
     can strbuf_reset and use the same buffer each time.
Signed-off-by: default avatarJeff King <peff@peff.net>
Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
parent 0bb443fd
...@@ -25,7 +25,7 @@ struct throughput { ...@@ -25,7 +25,7 @@ struct throughput {
unsigned int last_bytes[TP_IDX_MAX]; unsigned int last_bytes[TP_IDX_MAX];
unsigned int last_misecs[TP_IDX_MAX]; unsigned int last_misecs[TP_IDX_MAX];
unsigned int idx; unsigned int idx;
char display[32]; struct strbuf display;
}; };
struct progress { struct progress {
...@@ -98,7 +98,7 @@ static int display(struct progress *progress, unsigned n, const char *done) ...@@ -98,7 +98,7 @@ static int display(struct progress *progress, unsigned n, const char *done)
} }
progress->last_value = n; progress->last_value = n;
tp = (progress->throughput) ? progress->throughput->display : ""; tp = (progress->throughput) ? progress->throughput->display.buf : "";
eol = done ? done : " \r"; eol = done ? done : " \r";
if (progress->total) { if (progress->total) {
unsigned percent = n * 100 / progress->total; unsigned percent = n * 100 / progress->total;
...@@ -129,6 +129,7 @@ static int display(struct progress *progress, unsigned n, const char *done) ...@@ -129,6 +129,7 @@ static int display(struct progress *progress, unsigned n, const char *done)
static void throughput_string(struct strbuf *buf, off_t total, static void throughput_string(struct strbuf *buf, off_t total,
unsigned int rate) unsigned int rate)
{ {
strbuf_reset(buf);
strbuf_addstr(buf, ", "); strbuf_addstr(buf, ", ");
strbuf_humanise_bytes(buf, total); strbuf_humanise_bytes(buf, total);
strbuf_addstr(buf, " | "); strbuf_addstr(buf, " | ");
...@@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t total) ...@@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t total)
struct throughput *tp; struct throughput *tp;
uint64_t now_ns; uint64_t now_ns;
unsigned int misecs, count, rate; unsigned int misecs, count, rate;
struct strbuf buf = STRBUF_INIT;
if (!progress) if (!progress)
return; return;
...@@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t total) ...@@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t total)
if (tp) { if (tp) {
tp->prev_total = tp->curr_total = total; tp->prev_total = tp->curr_total = total;
tp->prev_ns = now_ns; tp->prev_ns = now_ns;
strbuf_init(&tp->display, 0);
} }
return; return;
} }
...@@ -193,9 +194,7 @@ void display_throughput(struct progress *progress, off_t total) ...@@ -193,9 +194,7 @@ void display_throughput(struct progress *progress, off_t total)
tp->last_misecs[tp->idx] = misecs; tp->last_misecs[tp->idx] = misecs;
tp->idx = (tp->idx + 1) % TP_IDX_MAX; tp->idx = (tp->idx + 1) % TP_IDX_MAX;
throughput_string(&buf, total, rate); throughput_string(&tp->display, total, rate);
strncpy(tp->display, buf.buf, sizeof(tp->display));
strbuf_release(&buf);
if (progress->last_value != -1 && progress_update) if (progress->last_value != -1 && progress_update)
display(progress, progress->last_value, NULL); display(progress, progress->last_value, NULL);
} }
...@@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) ...@@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1); bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
if (tp) { if (tp) {
struct strbuf strbuf = STRBUF_INIT;
unsigned int rate = !tp->avg_misecs ? 0 : unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs; tp->avg_bytes / tp->avg_misecs;
throughput_string(&strbuf, tp->curr_total, rate); throughput_string(&tp->display, tp->curr_total, rate);
strncpy(tp->display, strbuf.buf, sizeof(tp->display));
strbuf_release(&strbuf);
} }
progress_update = 1; progress_update = 1;
sprintf(bufp, ", %s.\n", msg); sprintf(bufp, ", %s.\n", msg);
...@@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const char *msg) ...@@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
free(bufp); free(bufp);
} }
clear_progress_signal(); clear_progress_signal();
if (progress->throughput)
strbuf_release(&progress->throughput->display);
free(progress->throughput); free(progress->throughput);
free(progress); free(progress);
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment