From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754144AbbIPQeH (ORCPT ); Wed, 16 Sep 2015 12:34:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35543 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753978AbbIPQeE (ORCPT ); Wed, 16 Sep 2015 12:34:04 -0400 From: Vitaly Kuznetsov To: Andy Shevchenko Cc: Rasmus Villemoes , Andrew Morton , James Bottomley , linux-kernel@vger.kernel.org, "K. Y. Srinivasan" Subject: Re: [PATCH v4 2/2] lib/test-string_helpers.c: add string_get_size() tests References: <1442325322-23366-1-git-send-email-vkuznets@redhat.com> <1442325322-23366-3-git-send-email-vkuznets@redhat.com> <1442330454.8361.47.camel@linux.intel.com> <87h9mu4nkb.fsf@rasmusvillemoes.dk> <1442403187.8361.62.camel@linux.intel.com> Date: Wed, 16 Sep 2015 18:34:01 +0200 In-Reply-To: <1442403187.8361.62.camel@linux.intel.com> (Andy Shevchenko's message of "Wed, 16 Sep 2015 14:33:07 +0300") Message-ID: <87wpvqwcg6.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andy Shevchenko 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'; >> >> 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.. > > Got your point. I also wanted to avoid the comment as it is self-explanatory. -- Vitaly