On 2017/9/5 下午12:19, Kent Overstreet wrote:

On Tue, Sep 05, 2017 at 10:50:51AM +0800, Coly Li wrote:

quoted

On 2017/9/5 上午5:55, Michael Lyle wrote:

quoted

Most importantly, solve a crash where %llu was used to format signed
numbers.  This would cause a buffer overflow when reading sysfs
writeback_rate_debug, as only 20 bytes were allocated for this and
%llu writes 20 characters plus a null.

Always use the units mechanism rather than having different output
paths for simplicity.

Also, correct problems with display output where 1.10 was a larger
number than 1.09, by multiplying by 10 and then dividing by 1024 instead
of dividing by 100.  (Remainders of >= 1000 would print as .10).

Minor changes: Always display the decimal point instead of trying to
omit it based on number of digits shown.  Decide what units to use
based on 1000 as a threshold, not 1024 (in other words, always print
at most 3 digits before the decimal point).

Signed-off-by: Michael Lyle <redacted>
Reported-by: Dmitry Yu Okunev <redacted>
---
 drivers/md/bcache/util.c | 50 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/md/bcache/util.c b/drivers/md/bcache/util.c
index 8c3a938f4bf0..176d3c2ef5f5 100644
--- a/drivers/md/bcache/util.c
+++ b/drivers/md/bcache/util.c

@@ -74,24 +74,44 @@ STRTO_H(strtouint, unsigned int)
 STRTO_H(strtoll, long long)
 STRTO_H(strtoull, unsigned long long)
 
+/**
+ * bch_hprint() - formats @v to human readable string for sysfs.
+ *
+ * @v - signed 64 bit integer
+ * @buf - the (at least 8 byte) buffer to format the result into.
+ *
+ * Returns the number of bytes used by format.
+ */
 ssize_t bch_hprint(char *buf, int64_t v)
 {
 	static const char units[] = "?kMGTPEZY";
-	char dec[4] = "";
-	int u, t = 0;
-
-	for (u = 0; v >= 1024 || v <= -1024; u++) {
-		t = v & ~(~0 << 10);
-		v >>= 10;
-	}
-
-	if (!u)
-		return sprintf(buf, "%llu", v);
-
-	if (v < 100 && v > -100)
-		snprintf(dec, sizeof(dec), ".%i", t / 100);
-
-	return sprintf(buf, "%lli%s%c", v, dec, units[u]);
+	int u = 0, t;
+
+	uint64_t q;

It would be good to remove a blank line between the variables.

I'd prefer without the blank line, but this is pretty nitpicky.

quoted

quoted

+
+	if (v < 0)
+		q = -v;
+	else
+		q = v;
+
+	/* For as long as the number is more than 3 digits, but at least
+	 * once, shift right / divide by 1024.  Keep the remainder for
+	 * a digit after the decimal point.
+	 */
+	do {
+		u++;
+
+		t = q & ~(~0 << 10);
+		q >>= 10;
+	} while (q >= 1000);
+

The original for-loop is correct, and the above do-while loop is
probably wrong. If q < 1024, a number without K/M/G/T/P/E/Z/Y should be
printed out, in this patch it is missing. And while (q>=1000) is not
correct, it should be (q>=1024), because >>10 means write shifting 10
bits, which is 1024 in decimal integer. How about just keep the
following original code,

It's just a style thing - printing out 100 vs 0.9k. I personally don't care one
way or the other.

Hi Kent,

Sure, I agree. You are the original author, once you are OK with current
patch, I don't have comment ^_^

quoted

	for (u = 0; v >= 1024 || v <= -1024; u++) {
		t = v & ~(~0 << 10);
		v >>= 10;
	}

	if (!u)
		return sprintf(buf, "%llu", v);
It is good enough IMHO.

quoted

+	if (v < 0)
+		/* '-', up to 3 digits, '.', 1 digit, 1 character, null;
+		 * yields 8 bytes.
+		 */
+		return sprintf(buf, "-%llu.%i%c", q, t * 10 / 1024, units[u]);
+	else
+		return sprintf(buf, "%llu.%i%c", q, t * 10 / 1024, units[u]);
 }

If you use char sign[2], that's two bytes temporary space on stack. Here
you use "-%llu.%i%c" and "%llu.%i%c" which occupy permanent space in
kernel readonly data section, which means 9 bytes more. That's not a big
memory consumption, but we can avoid it, why not.

The logic in this patch is much clear, and thanks for your detailed
commit log and patch comments. Could you please compose another version ?

The patch is to fix a rather serious bug where with small negative numbers it
blows through the output buffer. Let's get this in, please.

OK, I give up the paranoid. This patch will be in my patch list and sent
out in this merge window. Thanks for your response.

Coly Li