{"thread":{"id":"39660","subject":"[PATCH] progress: store throughput display in a strbuf","startedAt":"2015-06-17T18:54:06Z","lastAt":"2015-06-17T18:54:06Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264054","messageId":"20150617185406.GA727@peff.net","threadId":"39660","inReplyTo":null,"subject":"[PATCH] progress: store throughput display in a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T18:54:06Z","receivedAt":"2015-06-17T18:54:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Coverity noticed that we strncpy() into a fixed-size buffer\nwithout making sure that it actually ended up\nNUL-terminated. This is unlikely to be a bug in practice,\nsince throughput strings rarely hit 32 characters, but it\nwould be nice to clean it up.\n\nThe most obvious way to do so is to add a NUL-terminator.\nBut instead, this patch switches the fixed-size buffer out\nfor a strbuf. At first glance this seems much less\nefficient, until we realize that filling in the fixed-size\nbuffer is done by writing into a strbuf and copying the\nresult!\n\nBy writing straight to the buffer, we actually end up more\nefficient:\n\n  1. We avoid an extra copy of the bytes.\n\n  2. Rather than malloc/free each time progress is shown, we\n     can strbuf_reset and use the same buffer each time.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n progress.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 2e31bec..a3efcfd 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -25,7 +25,7 @@ struct throughput {\n \tunsigned int last_bytes[TP_IDX_MAX];\n \tunsigned int last_misecs[TP_IDX_MAX];\n \tunsigned int idx;\n-\tchar display[32];\n+\tstruct strbuf display;\n };\n \n struct progress {\n@@ -98,7 +98,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n \t}\n \n \tprogress->last_value = n;\n-\ttp = (progress->throughput) ? progress->throughput->display : \"\";\n+\ttp = (progress->throughput) ? progress->throughput->display.buf : \"\";\n \teol = done ? done : \"   \\r\";\n \tif (progress->total) {\n \t\tunsigned percent = n * 100 / progress->total;\n@@ -129,6 +129,7 @@ static int display(struct progress *progress, unsigned n, const char *done)\n static void throughput_string(struct strbuf *buf, off_t total,\n \t\t\t      unsigned int rate)\n {\n+\tstrbuf_reset(buf);\n \tstrbuf_addstr(buf, \", \");\n \tstrbuf_humanise_bytes(buf, total);\n \tstrbuf_addstr(buf, \" | \");\n@@ -141,7 +142,6 @@ void display_throughput(struct progress *progress, off_t total)\n \tstruct throughput *tp;\n \tuint64_t now_ns;\n \tunsigned int misecs, count, rate;\n-\tstruct strbuf buf = STRBUF_INIT;\n \n \tif (!progress)\n \t\treturn;\n@@ -154,6 +154,7 @@ void display_throughput(struct progress *progress, off_t total)\n \t\tif (tp) {\n \t\t\ttp->prev_total = tp->curr_total = total;\n \t\t\ttp->prev_ns = now_ns;\n+\t\t\tstrbuf_init(&tp->display, 0);\n \t\t}\n \t\treturn;\n \t}\n@@ -193,9 +194,7 @@ void display_throughput(struct progress *progress, off_t total)\n \ttp->last_misecs[tp->idx] = misecs;\n \ttp->idx = (tp->idx + 1) % TP_IDX_MAX;\n \n-\tthroughput_string(&buf, total, rate);\n-\tstrncpy(tp->display, buf.buf, sizeof(tp->display));\n-\tstrbuf_release(&buf);\n+\tthroughput_string(&tp->display, total, rate);\n \tif (progress->last_value != -1 && progress_update)\n \t\tdisplay(progress, progress->last_value, NULL);\n }\n@@ -250,12 +249,9 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \n \t\tbufp = (len < sizeof(buf)) ? buf : xmalloc(len + 1);\n \t\tif (tp) {\n-\t\t\tstruct strbuf strbuf = STRBUF_INIT;\n \t\t\tunsigned int rate = !tp->avg_misecs ? 0 :\n \t\t\t\t\ttp->avg_bytes / tp->avg_misecs;\n-\t\t\tthroughput_string(&strbuf, tp->curr_total, rate);\n-\t\t\tstrncpy(tp->display, strbuf.buf, sizeof(tp->display));\n-\t\t\tstrbuf_release(&strbuf);\n+\t\t\tthroughput_string(&tp->display, tp->curr_total, rate);\n \t\t}\n \t\tprogress_update = 1;\n \t\tsprintf(bufp, \", %s.\\n\", msg);\n@@ -264,6 +260,8 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)\n \t\t\tfree(bufp);\n \t}\n \tclear_progress_signal();\n+\tif (progress->throughput)\n+\t\tstrbuf_release(&progress->throughput->display);\n \tfree(progress->throughput);\n \tfree(progress);\n }\n-- \n2.4.4.719.g3984bc6\n"}]}