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;
Date:   Mon, 3 Jul 2023 11:56:27 +0200
From:   Willy Tarreau <w@1wt.eu>
To:     Zhangjin Wu <falcon@tinylab.org>
Cc:     arnd@arndb.de, david.laight@aculab.com,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-riscv@lists.infradead.org, linux@weissschuh.net,
        thomas@t-8ch.de
Subject: Re: [PATCH v5 14/14] selftests/nolibc: add mmap and munmap test cases
Message-ID: <ZKKbS3cwKcHgnGwu@1wt.eu>
References: <ZKJ35DyPOG+LAy5j@1wt.eu>
 <20230703080647.491363-1-falcon@tinylab.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230703080647.491363-1-falcon@tinylab.org>
Precedence: bulk

On Mon, Jul 03, 2023 at 04:06:47PM +0800, Zhangjin Wu wrote:
> > >     /* 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]

Then we have a problem somewhere else and the test should be debugger
instead. Are you sure there isn't a successful chdir() test before it
for example, that would change the directory ? If so maybe we just need
to save the current dir before calling it and restore it later.

> 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.

Same as above.

> I'm not sure if the other syscalls require an absolute path, so, the
> realpath() is called in this proposed method.

No, please do not overengineer tests. That's only hiding the dust under
the carpet and people adding more tests later that will randomly fail
will have a very hard time trying to figure what's happening under the
hood. If a test doesn't work as expected, we must not try to work around
it, but arrange to fix it.

Thanks,
Willy