From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753356AbbI1Uz3 (ORCPT ); Mon, 28 Sep 2015 16:55:29 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:35923 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753176AbbI1UzH (ORCPT ); Mon, 28 Sep 2015 16:55:07 -0400 From: Rasmus Villemoes To: Andy Shevchenko Cc: Andrew Morton , Tejun Heo , linux-kernel@vger.kernel.org, Kees Cook Subject: Re: [PATCH 4/4] test_printf: test printf family at runtime Organization: D03 References: <1443202865-25533-1-git-send-email-linux@rasmusvillemoes.dk> <1443202865-25533-5-git-send-email-linux@rasmusvillemoes.dk> <1443431579.8361.234.camel@linux.intel.com> X-Hashcash: 1:20:150928:andriy.shevchenko@linux.intel.com::NsfxZm9IUoe+lhz6:00000000000000000000000000001xtV X-Hashcash: 1:20:150928:keescook@chromium.org::41h0chAbe2EOxydu:00000000000000000000000000000000000000003nUm X-Hashcash: 1:20:150928:tj@kernel.org::m1JSAfaIZ5qePVO3:00006B7P X-Hashcash: 1:20:150928:akpm@linux-foundation.org::uoWgnznxGHNFfHUe:0000000000000000000000000000000000006XEL X-Hashcash: 1:20:150928:linux-kernel@vger.kernel.org::bfrLE63aa9jrqDyh:0000000000000000000000000000000006prD Date: Mon, 28 Sep 2015 22:55:04 +0200 In-Reply-To: <1443431579.8361.234.camel@linux.intel.com> (Andy Shevchenko's message of "Mon, 28 Sep 2015 12:12:59 +0300") Message-ID: <874miefemv.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 28 2015, Andy Shevchenko wrote: > On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote: >> This adds a simple module for testing the kernel's printf >> facilities. Previously, some %p extensions have caused a wrong return >> value in case the entire output didn't fit and/or been unusable in >> kasprintf(). This should help catch such issues. Also, it should help >> ensure that changes to the formatting algorithms don't break >> anything. >> >> I'm not sure if we have a struct dentry or struct file lying around >> at >> boot time or if we can fake one, but most %p extensions should be >> testable, as should the ordinary number and string formatting. >> >> The nature of vararg functions means we can't use a more conventional >> table-driven approach. >> >> For now, this is mostly a skeleton; contributions are very >> welcome. Some tests are/will be slightly annoying to write, since the >> expected output depends on stuff like CONFIG_*, sizeof(long), runtime >> values etc. > > Few comments below. > >> + >> +#define test(expect, fmt, ...) \ >> + __test(expect, strlen(expect), fmt, ##__VA_ARGS__) > > Would be __test_m[em] / __test_s[tr] to distinguish them by name? Erh, no. The 'mem' version will only be used in a very few cases, and I really want the simple name "test" for the common case. > And might be inline function? That'd make the vararg handling more cumbersome. >> +static void __init >> +test_basic(void) >> +{ >> + test("", ""); >> + test("100%", "100%%"); >> + test("xxx%yyy", "xxx%cyyy", '%'); >> + __test("xxx\0yyy", 7, "xxx%cyyy", '\0'); > > And such pieces will be look better > > __test_str("xxx%yyy", "xxx%cyyy", '%'); > __test_mem("xxx\0yyy", 7, "xxx%cyyy", '\0'); I don't agree. >> + >> +static void __init >> +netdev_features(void) >> +{ >> +} >> + >> + > > Maybe commentary delimiter here and above where you have double empty > line. And say what? I can avoid double empty lines if they bother you. >> + >> + return 0; > > Do we need this module in a memory? I guess not. At first I thought it didn't really matter since all functions and data are __init, but I suppose a little metadata would stick around if loading is "successful". Will fix. >> + >> +MODULE_AUTHOR("Rasmus Villemoes "); >> +MODULE_LICENSE("GPL"); > > GPL or ?.. Honestly, I don't really care. Would you like BSD/GPL or what? I just copied from the majority of MODULE_LICENSE() instances. Rasmus