Andy Shevchenko <andriy.shevche...@linux.intel.com> writes:
> On Wed, 2015-09-16 at 13:21 +0200, Rasmus Villemoes wrote:
>> On Tue, Sep 15 2015, Andy Shevchenko <
>> andriy.shevche...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
