From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D2ECEEB64DD for ; Mon, 3 Jul 2023 09:15:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=BXKSeQ+CzCDFd/e/bZuRNnhv1znUPOQl45beCnFhjzs=; b=L9Dfqelur91liN ratZEs7gQW0TdkMU33NgDxJMTNxBLpdsRncRB23C/PaqSLb7bldiKFDWP8nUAx9rkrGhUgmFa8Lp+ S3IVQnpMZ4NSTQ8d+EZUKSo1HMpgRSRQZeBVfOftTDCcuPDdQvgm0kh6sllgsiUI1BjGucrWfkwB5 pEvFnY/6PrjSvS9ab9l9FWQJHnZJrJnUZXtA1kxWTRBE7h7E3atZ3K9DMnyfZUYcfro1EiJnbQxJi WCGM60h2Fz23EnFqAHyOmSKKJms/jn4pktzJE2xYZHW4UYWHiwizjyLHYUhRPN3yjTlXT0WtSKTkW KcTfE+XwcQ0X9UJstiGg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qGFeu-009tId-38; Mon, 03 Jul 2023 09:15:48 +0000 Received: from bg4.exmail.qq.com ([43.154.221.58]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qGFer-009tGZ-0R for linux-riscv@lists.infradead.org; Mon, 03 Jul 2023 09:15:47 +0000 X-QQ-mid: bizesmtp75t1688375722th1fa91f Received: from linux-lab-host.localdomain ( [119.123.131.49]) by bizesmtp.qq.com (ESMTP) with id ; Mon, 03 Jul 2023 17:15:21 +0800 (CST) X-QQ-SSF: 01200000000000D0W000000A0000000 X-QQ-FEAT: znfcQSa1hKYmGKLly0cpa018jyPZvn3QolrVhbDV/s3KpSQEDu1YDAdsEzZ/o LkRnAFX7Lxz9jbfIysjrjg4aO/OZQBdgDXx4M3O34Rl3DEXpZddlJqsoD9nrIsSSYVnrwPb Cn5XtmE9rRgU9C6xHuNm7TEDMo4V2tGOAjWMCbjehq6tECvb5Il4fmeZZ5EYWkVlfpvVdZ+ NwnFit1uYxWe9BM+Tus0ea5+bDAe29hWH2QkhE/3mE1hEdOzTSBecFC9Bifxp89kPRC5aSi aAmf+vrVoqcs3wGryg9+GZ61egO03PyFP8k3jy2N3jPs4IemQCe5KavPtDNr2DbnbqT6R5a 0J2Cr5kh5MVov5C96VkBPaJRLophe+aMRdmU8nomYs/eH7uqfg= X-QQ-GoodBg: 0 X-BIZMAIL-ID: 16105542733624423556 From: Zhangjin Wu To: thomas@t-8ch.de Cc: arnd@arndb.de, david.laight@aculab.com, falcon@tinylab.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, linux-riscv@lists.infradead.org, w@1wt.eu Subject: Re: [PATCH v5 14/14] selftests/nolibc: add mmap and munmap test cases Date: Mon, 3 Jul 2023 17:15:21 +0800 Message-Id: <20230703091521.492045-1-falcon@tinylab.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrgz:qybglogicsvrgz5a-1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230703_021545_498152_3D58CEDE X-CRM114-Status: GOOD ( 39.93 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org Hi, Thomas > On 2023-07-03 16:06:47+0800, Zhangjin Wu wrote: > > Hi, Willy > > > [..] > > > > > - argv[0] > > > > > > > > since nolibc has no realpath() currently, we can simply > > > > support the current path and the absolute path like this: > > > > > > > > nolibc-test.c: > > > > > > > > /* assigned as argv[0] in main(), will be used by some tests */ > > > > static char exe[PATH_MAX + 1]; > > > > > > > > main(): > > > > > > > > /* get absolute path of myself, nolibc has no realpath() currently */ > > > > #ifndef NOLIBC > > > > realpath(argv[0], exe); > > > > #else > > > > /* assume absolute path has no "./" */ > > > > if (strncmp(argv[0], "./", 2) != 0) > > > > strncat(exe, argv[0], strlen(argv[0]) + 1); > > > > else { > > > > pwd = getenv("PWD"); > > > > /* skip the ending '\0' */ > > > > strncat(exe, getenv("PWD"), strlen(pwd)); > > > > /* skip the first '.' */ > > > > strncat(exe, argv[0] + 1, strlen(argv[0])); > > > > } > > > > #endif > > > > > > No, please, not like this. Just copy argv[0] (the pointer not the > > > contents) and you're fine: > > > > > > static const char *argv0; > > > > > > int main(int argc, char **argv, char **envp) > > > { > > > argv0 = argv[0]; > > > ... > > > } > > > > > > Nothing more, nothing less. Your program will always have its correct > > > path when being called unless someone purposely forces it to something > > > different, which is not our concern at all since this is a test program. > > > And I'd rather call it "argv0" which exactly tells us what it contains > > > than "exe" which can be misleading for that precise reason. > > > > > > > Yeah, locally, I just used a global argv0 pointer directly, but > > chroot_exe("./nolibc-test") not work when run 'libc-test' in host > > system, that is why I tried to get an absolute path ;-) > > > > CASE_TEST(chroot_exe); EXPECT_SYSER(1, chroot(exe), -1, ENOTDIR); break; > > > > --> > > > > 19 chroot_exe = -1 ENOENT != (-1 ENOTDIR) [FAIL] > > > > I removed the "proc ?" check manually to test if it also work with > > CONFIG_PROC_FS=n. it doesn't work, without absolute path, we need to add > > the ENOENT errno back to the errno check list. > > > > I'm not sure if the other syscalls require an absolute path, so, the > > realpath() is called in this proposed method. > > > > > > A full functional realpath() is a little complex, such as '../' support and > > > > even symlink support, let's delay its requirement at current stage ;-) > > > > > > Please do not even engage into this, and keep in mind that the sole > > > purpose of this test program is to help developers simply add tests to > > > the set of existing ones. If the program becomes complex for doing stuff > > > that is out of its scope, it will become much harder to extend and users > > > will lose interest and motivation for updating it. > > > > > > > one or both of them may also help the other test cases: > > > > > > > > - chroot_exe (used '/init' before) > > > > > > > > CASE_TEST(chroot_exe); EXPECT_SYSER(1, chroot(proc ? "/proc/self/exe" : exe), -1, ENOTDIR); break; > > > > > > > > - chmod_exe (replace the one: chmod_tmpdir in another patchset) > > > > > > > > CASE_TEST(chmod_exe); EXPECT_SYSZR(1, chmod(proc ? "/proc/self/exe" : exe, 0555)); break; > > > > > > > > It should be safe enough to only remove the writable attribute for the test > > > > program. > > > > > > > > - stat_timestamps (used '/init' before) > > > > > > > > if (stat("/proc/self/", &st) && stat(exe, &st) && stat("/dev/zero", &st) && stat("/", &st)) > > > > > > Indeed, why not! > > > > > > > Ok, without absolute path, the chroot_exe() will be changed back to > > something like this: > > > > CASE_TEST(chroot_exe); EXPECT_SYSER2(1, chroot(proc ? "/proc/self/exe" : argv0), -1, ENOTDIR, ENOENT); break; > > Are you sure the ENOENT is really correct? > I played with this before and got ENOENT because before the chroot test > we have a testcase that does chdir("/"). Yes, there are some chdir tests before chroot, it does answer why relative path not work and return ENOENT: no such file in the relative path changed by chdir(), it differs from the one in PWD environment variable. > And therefore the relative name in argv[0] was not resolving correctly > anymore against the changed working directory. > > (You can also test this by executing *only* the chroot test and it > should work) > Yeah, If chdir() back to current path, it does work: CASE_TEST(chroot_exe); chdir(getenv("PWD")); EXPECT_SYSER(1, chroot(exe), -1, ENOTDIR); break; --> 11 chdir_root = 0 [OK] 12 chdir_dot = 0 [OK] 13 chdir_blah = -1 ENOENT [OK] 14 chmod_self = -1 EPERM [OK] 15 chmod_exe = 0 [OK] 16 chown_self = -1 EPERM [OK] 17 chroot_root [SKIPPED] 18 chroot_blah = -1 ENOENT [OK] 19 chroot_exe pwd: /home/ubuntu/Develop/src/examples/musl exe: ./nolibc-test = -1 ENOTDIR [OK] > In general chroot() should work just fine with relative paths. > it does work with relative path, to make sure argv0 always work as expected, an extra 'chdir()' may really required, or let's clean up the previous chdir() test cases to call chdir(getenv("PWD")) after every test. CASE_TEST(chdir_root); EXPECT_SYSZR(1, chdir("/")); break; CASE_TEST(chdir_dot); EXPECT_SYSZR(1, chdir(".")); break; CASE_TEST(chdir_blah); EXPECT_SYSER(1, chdir("/blah"), -1, ENOENT); break; perhaps only call 'chdir(getenv("PWD"))' after chdir_root() is enough, because the chdir(".") doesn't really change the directory: CASE_TEST(chdir_root); EXPECT_SYSZR(1, chdir("/")); chdir(getenv("PWD")); break; CASE_TEST(chdir_dot); EXPECT_SYSZR(1, chdir(".")); break; CASE_TEST(chdir_blah); EXPECT_SYSER(1, chdir("/blah"), -1, ENOENT); break; Which one do you prefer? > This is really a lot of complexity and discussion only to avoid > depending on procfs for the tests. > Yes, one step further to find more interesting info, thanks a lot ;-) Mixing chdir and relative path really need to be more careful. Best regards, Zhangjin > Thomas _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv