Messages in this thread | | | From | Vitaly Kuznetsov <> | Subject | Re: [PATCH v4 2/2] lib/test-string_helpers.c: add string_get_size() tests | Date | Wed, 16 Sep 2015 18:34:01 +0200 |
| |
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
| |