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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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

> On Wed, 2015-09-16 at 13:21 +0200, Rasmus Villemoes wrote:
>> On Tue, Sep 15 2015, Andy Shevchenko <
>> andriy.shevchenko@linux.intel.com> wrote:
>> 
>> > On Tue, 2015-09-15 at 15:55 +0200, Vitaly Kuznetsov 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[16];
>> > > +
>> > > +	string_get_size(size, blk_size, units, buf, 
>> > > sizeof(buf));
>> > > +	if (!memcmp(buf, exp_result, strnlen(exp_result, 
>> > > sizeof(buf) 
>> > > - 1) + 1))
>> > 
>> > Actually you don't need to do this +- 1. Either you will have '\0' 
>> > or
>> > not, it will be checked by memcmp() anyway.
>> > 
>> > Thus,
>> > memcmp(buf, exp_result, strnlen(exp_result, sizeof(buf))).
>> 
>> Huh? How does that ensure that string_get_size put a '\0' at the 
>> right
>> spot? We do need the comparison to also cover the terminating '\0' in
>> exp_result.
>
> Ah, you are right.
>
> But seems we have length of the exp_result always smaller than buffer
> size, so, would we change this to 
>  memcmp(…, strlen(exp_result) + 1);
> ?
>
>> [It would be nice if we could assert at compile-time that
>> strlen(exp_result) < sizeof(buf).]
>
> Interesting if BUILD_BUG_ON can help here. Can we use
> sizeof(exp_result) since all of them are literal constants?
>

Yes it can. The following seems to be working for me:

+#define string_get_size_maxbuf 16
+#define test_string_get_size_one(size, blk_size, units, exp_result)            \
+       do {                                                                   \
+               BUILD_BUG_ON(sizeof(exp_result) >= string_get_size_maxbuf);    \
+               __test_string_get_size((size), (blk_size), (units),            \
+                                      (exp_result));                          \
+       } while(0);
+
+
+static __init void __test_string_get_size(u64 size, u64 blk_size,
+                                         const enum string_size_units units,
+                                         const char *exp_result)
+{
+       char buf[string_get_size_maxbuf];
+
+       string_get_size(size, blk_size, units, buf, sizeof(buf));
+       if (!memcmp(buf, exp_result, strlen(exp_result) + 1))
+               return;
+
+       buf[sizeof(buf) - 1] = '\0';
+       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);
+}

>> 
>> > Perhaps one line comment here
>> > /* Make sure that buf will be always NULL-terminated */
>> > 
>> > > +	buf[sizeof(buf) - 1] = '\0';
>> 
>> <bikeshed>Could we pretty-please use different names for 0 the 
>> pointer
>> and 0 the character, say in this case nul or NUL or '\0' or simply
>> 0. Also, I don't see the value of the comment; that line is a totally
>> standard idiom.</bikeshed>.
>
> Got your point.

I also wanted to avoid the comment as it is self-explanatory.

-- 
  Vitaly