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

[Petr Vorel <pvorel@suse.cz>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp