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: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/progress.c b/progress.c
index 2e31bec60f5c98dc7dac69227759cbf2ba5f9974..a3efcfd34ccf60bcd41d7d2aef18fcb2aeca7837 100644 (file)
--- a/progress.c
+++ b/progress.c
@@ -25,7 +25,7 @@ struct throughput {
        unsigned int last_bytes[TP_IDX_MAX];
        unsigned int last_misecs[TP_IDX_MAX];
        unsigned int idx;
        unsigned int last_bytes[TP_IDX_MAX];
        unsigned int last_misecs[TP_IDX_MAX];
        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)
        }
 
        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";
        if (progress->total) {
                unsigned percent = n * 100 / progress->total;
        eol = done ? done : "   \r";
        if (progress->total) {
                unsigned percent = n * 100 / progress->total;


@@ -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,
                              unsigned int rate)
 {
 static void throughput_string(struct strbuf *buf, off_t total,
                              unsigned int rate)
 {

+       strbuf_reset(buf);

        strbuf_addstr(buf, ", ");
        strbuf_humanise_bytes(buf, total);
        strbuf_addstr(buf, " | ");
        strbuf_addstr(buf, ", ");
        strbuf_humanise_bytes(buf, total);
        strbuf_addstr(buf, " | ");


@@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t total)
        struct throughput *tp;
        uint64_t now_ns;
        unsigned int misecs, count, rate;
        struct throughput *tp;
        uint64_t now_ns;
        unsigned int misecs, count, rate;

-       struct strbuf buf = STRBUF_INIT;

 
        if (!progress)
                return;
 
        if (!progress)
                return;


@@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t total)
                if (tp) {
                        tp->prev_total = tp->curr_total = total;
                        tp->prev_ns = now_ns;
                if (tp) {
                        tp->prev_total = tp->curr_total = total;
                        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)
        tp->last_misecs[tp->idx] = misecs;
        tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 
        tp->last_misecs[tp->idx] = misecs;
        tp->idx = (tp->idx + 1) % TP_IDX_MAX;
 

-       throughput_string(&buf, total, rate);
-       strncpy(tp->display, buf.buf, sizeof(tp->display));
-       strbuf_release(&buf);
+       throughput_string(&tp->display, total, rate);

        if (progress->last_value != -1 && progress_update)
                display(progress, progress->last_value, NULL);
 }
        if (progress->last_value != -1 && progress_update)
                display(progress, progress->last_value, NULL);
 }


@@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 
                bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
                if (tp) {
 
                bufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);
                if (tp) {

-                       struct strbuf strbuf = STRBUF_INIT;

                        unsigned int rate = !tp->avg_misecs ? 0 :
                                        tp->avg_bytes / tp->avg_misecs;
                        unsigned int rate = !tp->avg_misecs ? 0 :
                                        tp->avg_bytes / tp->avg_misecs;

-                       throughput_string(&strbuf, tp->curr_total, rate);
-                       strncpy(tp->display, strbuf.buf, sizeof(tp->display));
-                       strbuf_release(&strbuf);
+                       throughput_string(&tp->display, tp->curr_total, rate);

                }
                progress_update = 1;
                sprintf(bufp, ", %s.\n", msg);
                }
                progress_update = 1;
                sprintf(bufp, ", %s.\n", msg);


@@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
                        free(bufp);
        }
        clear_progress_signal();
                        free(bufp);
        }
        clear_progress_signal();

+       if (progress->throughput)
+               strbuf_release(&progress->throughput->display);

        free(progress->throughput);
        free(progress);
 }
        free(progress->throughput);
        free(progress);
 }