Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1224318 > unrolled thread

[PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-09-14 18:50 +0200
    Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-15 00:10 +0200
      Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size()  tests Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-09-15 08:50 +0200
        Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size()  tests Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-09-15 14:20 +0200
        Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests Vitaly Kuznetsov <vkuznets@redhat.com> - 2015-09-15 14:20 +0200

#1224318 — [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests

Add a couple of simple tests for string_get_size(). The last one will hang
the kernel without the 'lib/string_helpers.c: fix infinite loop in
string_get_size()' fix.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 lib/test-string_helpers.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c
index 8e376ef..ee67ada 100644
--- a/lib/test-string_helpers.c
+++ b/lib/test-string_helpers.c
@@ -326,6 +326,30 @@ out:
 	kfree(out_test);
 }
 
+static __init void test_string_get_size_one(u64 size, u64 blk_size,
+					    const enum string_size_units units,
+					    const char *exp_result)
+{
+	char buf[256];
+
+	string_get_size(size, blk_size, units, buf, sizeof(buf));
+	if (!strncmp(buf, exp_result, min(sizeof(buf), strlen(exp_result))))
+		return;
+
+	pr_warn("Test 'test_string_get_size_one' failed!\n");
+	pr_warn("string_get_size(size = %llu, blk_size = %llu, units = %d\n",
+		size, blk_size, units);
+	pr_warn("expected: %s, got %s\n", exp_result, buf);
+
+}
+
+static __init void test_string_get_size(void)
+{
+	test_string_get_size_one(16384, 512, STRING_UNITS_2, "8.00 MiB");
+	test_string_get_size_one(8192, 4096, STRING_UNITS_10, "32.7 MB");
+	test_string_get_size_one(1, 512, STRING_UNITS_10, "512 B");
+}
+
 static int __init test_string_helpers_init(void)
 {
 	unsigned int i;
@@ -344,6 +368,9 @@ static int __init test_string_helpers_init(void)
 	for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++)
 		test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1);
 
+	/* Test string_get_size() */
+	test_string_get_size();
+
 	return -EINVAL;
 }
 module_init(test_string_helpers_init);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1224491

On Mon, Sep 14 2015, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> +static __init void test_string_get_size_one(u64 size, u64 blk_size,
> +					    const enum string_size_units units,
> +					    const char *exp_result)
> +{
> +	char buf[256];
> +
> +	string_get_size(size, blk_size, units, buf, sizeof(buf));
> +	if (!strncmp(buf, exp_result, min(sizeof(buf), strlen(exp_result))))
> +		return;

Nits: It probably makes sense to also test that string_get_size
'\0'-terminates the buffer, so I'd spell this

  if (!memcmp(buf, exp_result, min(sizeof(buf), strlen(exp_result)+1)))

With a generous stack buffer, that min() will always evaluate to the
strlen(exp_result)+1. On that note: Maybe 256 is a bit excessive. I
don't think this will run very deep in the kernel stack, but the code might
get copy-pasted somewhere else. 16 should be plenty.

> +	pr_warn("Test 'test_string_get_size_one' failed!\n");
> +	pr_warn("string_get_size(size = %llu, blk_size = %llu, units = %d\n",
> +		size, blk_size, units);

[There's probably no pretty way of getting from units to a text
representation, but it's slightly annoying to have to check the source
for the enum definition to figure out what units=0 or units=1 means.]

> +	pr_warn("expected: %s, got %s\n", exp_result, buf);

In case we failed to '\0'-terminate buf, we might want to print it with
"%.*s", (int)sizeof(buf), buf. But maybe I'm just overly paranoid.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1224670 — Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests

On Tue, 2015-09-15 at 00:00 +0200, Rasmus Villemoes wrote:
> On Mon, Sep 14 2015, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> 

Vitaly, thanks for the test cases. My comments below.

> > +static __init void test_string_get_size_one(u64 size, u64 
> > blk_size,
> > +					    const enum 
> > string_size_units units,
> > +					    const char 
> > *exp_result)
> > +{
> > +	char buf[256];
> > +
> > +	string_get_size(size, blk_size, units, buf, sizeof(buf));
> > +	if (!strncmp(buf, exp_result, min(sizeof(buf), 
> > strlen(exp_result))))
> > +		return;
> 
> Nits: It probably makes sense to also test that string_get_size
> '\0'-terminates the buffer, so I'd spell this
> 
>   if (!memcmp(buf, exp_result, min(sizeof(buf), 
> strlen(exp_result)+1)))
> 
> With a generous stack buffer, that min() will always evaluate to the
> strlen(exp_result)+1. On that note: Maybe 256 is a bit excessive. I
> don't think this will run very deep in the kernel stack, but the code 
> might
> get copy-pasted somewhere else. 16 should be plenty.

Agree with Rasmus.

And just to make a side note that useless use of min() since we have
strnlen() :-)

> 
> > +	pr_warn("Test 'test_string_get_size_one' failed!\n");
> > +	pr_warn("string_get_size(size = %llu, blk_size = %llu, 
> > units = %d\n",
> > +		size, blk_size, units);
> 
> [There's probably no pretty way of getting from units to a text
> representation, but it's slightly annoying to have to check the 
> source
> for the enum definition to figure out what units=0 or units=1 means.]
> 
> > +	pr_warn("expected: %s, got %s\n", exp_result, buf);
> 
> In case we failed to '\0'-terminate buf, we might want to print it 
> with
> "%.*s", (int)sizeof(buf), buf. But maybe I'm just overly paranoid.

I prefer to put '\0' at the position after we expected have an actual
'\0'. In this case we always be NULL terminated. I did this for hexdump
test cases.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225033 — Re: [PATCH v3 2/2] lib/test-string_helpers.c: add string_get_size() tests

On Tue, 2015-09-15 at 14:10 +0200, Vitaly Kuznetsov wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > I prefer to put '\0' at the position after we expected have an
> > actual
> > '\0'. In this case we always be NULL terminated. I did this for 
> > hexdump
> > test cases.
> 
> Just to check I got your suggestions right:
> 
> ...
> +       if (!memcmp(buf, exp_result, strnlen(exp_result, sizeof(buf) 
> - 1) + 1))
> +               return;
> +
> +       /* NULL terminate buf right after the expected '\0' */
> +       buf[strnlen(exp_result, sizeof(buf) - 2) + 1] = '\0';
> ...
> 
> Alternatively, we could have avoided strnlen() by asserting
> strlen(exp_result) < sizeof(buf) - 1 at the very beginning.
> 

Just 

buf[sizeof(buf) - 1] = '\0';

should be enough after you called the string_get_size().

And minimize the buffer to something like 16 (whatever is the biggest
possible length + '\0' + some space for the wrong algorithm aligned to
let's say 4.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1225035

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Tue, 2015-09-15 at 00:00 +0200, Rasmus Villemoes wrote:
>> On Mon, Sep 14 2015, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> 
>
> Vitaly, thanks for the test cases. My comments below.
>
>> > +static __init void test_string_get_size_one(u64 size, u64 
>> > blk_size,
>> > +					    const enum 
>> > string_size_units units,
>> > +					    const char 
>> > *exp_result)
>> > +{
>> > +	char buf[256];
>> > +
>> > +	string_get_size(size, blk_size, units, buf, sizeof(buf));
>> > +	if (!strncmp(buf, exp_result, min(sizeof(buf), 
>> > strlen(exp_result))))
>> > +		return;
>> 
>> Nits: It probably makes sense to also test that string_get_size
>> '\0'-terminates the buffer, so I'd spell this
>> 
>>   if (!memcmp(buf, exp_result, min(sizeof(buf), 
>> strlen(exp_result)+1)))
>> 
>> With a generous stack buffer, that min() will always evaluate to the
>> strlen(exp_result)+1. On that note: Maybe 256 is a bit excessive. I
>> don't think this will run very deep in the kernel stack, but the code 
>> might
>> get copy-pasted somewhere else. 16 should be plenty.
>
> Agree with Rasmus.
>
> And just to make a side note that useless use of min() since we have
> strnlen() :-)
>
>> 
>> > +	pr_warn("Test 'test_string_get_size_one' failed!\n");
>> > +	pr_warn("string_get_size(size = %llu, blk_size = %llu, 
>> > units = %d\n",
>> > +		size, blk_size, units);
>> 
>> [There's probably no pretty way of getting from units to a text
>> representation, but it's slightly annoying to have to check the 
>> source
>> for the enum definition to figure out what units=0 or units=1 means.]
>> 
>> > +	pr_warn("expected: %s, got %s\n", exp_result, buf);
>> 
>> In case we failed to '\0'-terminate buf, we might want to print it 
>> with
>> "%.*s", (int)sizeof(buf), buf. But maybe I'm just overly paranoid.
>
> I prefer to put '\0' at the position after we expected have an actual
> '\0'. In this case we always be NULL terminated. I did this for hexdump
> test cases.

Just to check I got your suggestions right:

...
+       if (!memcmp(buf, exp_result, strnlen(exp_result, sizeof(buf) - 1) + 1))
+               return;
+
+       /* NULL terminate buf right after the expected '\0' */
+       buf[strnlen(exp_result, sizeof(buf) - 2) + 1] = '\0';
...

Alternatively, we could have avoided strnlen() by asserting
strlen(exp_result) < sizeof(buf) - 1 at the very beginning.

-- 
  Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web