Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zhangjin Wu <falcon@tinylab.org>
To:     thomas@t-8ch.de
Cc:     falcon@tinylab.org, arnd@arndb.de, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, w@1wt.eu
Subject: Re: [PATCH v1 00/17] selftests/nolibc: allow run with minimal kernel config
Date:   Sat, 24 Jun 2023 16:54:03 +0800
Message-Id: <20230624085403.603469-1-falcon@tinylab.org>
In-Reply-To: <20230622184559.1188894-1-falcon@tinylab.org>
References: <20230622184559.1188894-1-falcon@tinylab.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrsz:qybglogicsvrsz3a-3
Precedence: bulk

Hi, Thomas

> > Hi Zhangjin,
> > 
> > some general comments for the whole series.
> > 
> > On 2023-06-21 20:52:30+0800, Zhangjin Wu wrote:
> > > Hi, Willy
> > > 
> > > This patchset mainly allows speed up the nolibc test with a minimal
> > > kernel config.
> > > 
> (snip)
> > > 
> > > * selftests/nolibc: fix up kernel parameters support
> > > 
> > >   kernel cmdline allows pass two types of parameters, one is without
> > >   '=', another is with '=', the first one is passed as init arguments,
> > >   the sencond one is passed as init environment variables.
> > > 
> > >   Our nolibc-test prefer arguments to environment variables, this not
> > >   work when users add such parameters in the kernel cmdline:
> > > 
> > >     noapic NOLIBC_TEST=syscall
> > > 
> > >   So, this patch will verify the setting from arguments at first, if it
> > >   is no valid, will try the environment variables instead.
> > 
> > This would be much simpler as:
> > 
> > test = getenv("NOLIBC_TEST");
> > if (!test)
> >         test = argv[1];
> >
> > It changes the semantics a bit, but it doesn't seem to be an issue.
> > (Maybe gated behind getpid() == 1).
> 
> Cool suggestion, it looks really better:
> 
> 	if (getpid() == 1) {
> 		prepare();
> 		
> 		/* kernel cmdline may pass: "noapic NOLIBC_TEST=syscall",
>                  * to init program:
> 		 *
> 		 *   "noapic" as arguments,
> 		 *   "NOLIBC_TEST=syscall" as environment variables,
>                  *
> 		 * to avoid getting null test in this case, parsing
> 		 * environment variables at first.
> 		 */
> 		test = getenv("NOLIBC_TEST");
> 		if (!test)
> 			test = argv[1];
> 	} else {
> 		/* for normal nolibc-test program, prefer arguments */
> 		test = argv[1];
> 		if (!test)
> 			test = getenv("NOLIBC_TEST");
> 	}
> 

Test shows, when no NOLIBC_TEST environment variable passed to kernel cmdline,
it will still branch to this code:

    test = argv[1]; /* nopaic ... */

And therefore report the whole test is ignored and no test will be run:

    Ignoring unknown test name 'noapic'

So, we may still need to verify it like my originally proposed method, but
let's further verify the one from NOLIBC_TEST=, we should tune the code a
litle.

Thanks,
Zhangjin