Re: [LTP] [PATCH v1] cpuset_memory_test.c: Use $TMPDIR as prefix for HUGEPAGE file path

Petr Vorel
2024-08-01T12:16:49Z

Hi Wei,

> Test case will fail with following error if running operation system
> which force root path read ONLY.

> mkdir: cannot create directory ‘/hugetlb’: Read-only file system

> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
> .../cpuset_memory_test/cpuset_memory_test.c | 11 ++++++++---
> .../cpuset_memory_test/cpuset_memory_testset.sh | 16 ++++++++--------
> 2 files changed, 16 insertions(+), 11 deletions(-)

> diff --git a/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_test.c b/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_test.c
> index 9912d8d6a..73770fd3c 100644
> --- a/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_test.c
> +++ b/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_test.c
> @@ -177,9 +177,14 @@ void mmap_file(int flag_allocated)
> static int fd_hugepage;
> int fd_tmp;

> + char path[100];
> + char *tmpdir = getenv("TMPDIR");
> +
> + sprintf(path, "%s%s", tmpdir, FILE_HUGEPAGE);

FYI, we have custom function, thus no need to detect TMPDIR in the test.

Because this is still the legacy API, could you please use tst_tmpdir_path()?

FYI in the new API We had for a long time tst_get_tmpdir() function needed to
use free() and was used for snprintf(), but Cyril changed that quite recently:
https://github.com/linux-test-project/ltp/commit/c5d95b6d34e2356bd19e6b646da06f1bce66a024

Since that just use tst_tmpdir_path(), no need to free.

IMHO both legacy API and new API do chdir() to the temporary directory, see
lib/tst_tmpdir.c:

if (chdir(TESTDIR) == -1) {
tst_resm(TERRNO, "%s: chdir(%s) failed", __func__, TESTDIR);

Therefore unless there is full path needed (e.g. search in /proc/mounts) we
could use relative path, e.g. add "./" to the definition:

+++ testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_test.c
@@ -58,7 +58,7 @@ static int opt_thread;
static int key_id; /* used with opt_shm */
static unsigned long memsize;

-#define FILE_HUGEPAGE "/hugetlb/hugepagefile"
+#define FILE_HUGEPAGE "./hugetlb/hugepagefile"

#define MMAP_ANON (SCHAR_MAX + 1)
#define MMAP_FILE (SCHAR_MAX + 2)
---

Also, the same definition is in
testcases/kernel/controllers/memcg/functional/memcg_process.c, is it affected as
well?

FYI Besides that it's always better to use 'n' sprintf functions (e.g.
snprintf(), because they restrict the write size (man printf(3): "write at most size bytes")

> +
> if (!flag_allocated) {
> if (opt_hugepage) {
> - fd_hugepage = open(FILE_HUGEPAGE,
> + fd_hugepage = open(path,
> O_CREAT | O_RDWR, 0755);
> if (fd_hugepage < 0)
> err(1, "open hugepage file failed");
> @@ -191,7 +196,7 @@ void mmap_file(int flag_allocated)
> MAP_SHARED, fd_tmp, 0);
> if (p == MAP_FAILED) {
> if (opt_hugepage)
> - unlink(FILE_HUGEPAGE);
> + unlink(path);
> err(1, "mmap(file) failed");
> }
> touch_memory_and_echo_node(p, memsize);
> @@ -201,7 +206,7 @@ void mmap_file(int flag_allocated)

> if (opt_hugepage) {
> close(fd_hugepage);
> - unlink(FILE_HUGEPAGE);
> + unlink(path);
> }
> }
> }
> diff --git a/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_testset.sh b/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_testset.sh
> index c1e7cea8f..b63425088 100755
> --- a/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_testset.sh
> +++ b/testcases/kernel/controllers/cpuset/cpuset_memory_test/cpuset_memory_testset.sh
> @@ -175,8 +175,8 @@ test6()
> return 0
> fi

> - mkdir /hugetlb
> - mount -t hugetlbfs none /hugetlb
> + mkdir ${TMPDIR}/hugetlb
Could you please use $TMPDIR instead of ${TMPDIR}? (more readable).

> + mount -t hugetlbfs none ${TMPDIR}/hugetlb
And here (there are more places).

BTW just ./hugetlb could work, right? Also, in the shell API we are in the
directory.

And thanks for pointing these tests, once Cyril's effort for mixing C and shell
code is merged, we should adapt these tests to use it.

https://patchwork.ozlabs.org/project/ltp/list/?series=417372&state=*

Kind regards,
Petr