Re: [LTP] [PATCH v2] Add rename15 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

...
> +++ b/runtest/smoketest
> @@ -10,7 +10,7 @@ write01 write01
>  symlink01 symlink01
>  stat04 symlink01 -T stat04
>  utime01A symlink01 -T utime01
> -rename01A symlink01 -T rename01
> +rename15 rename15
FYI this needs to be rebased now.

>  splice02 splice02 -s 20
>  df01_sh df01.sh
>  shell_test01 echo "SUCCESS" | shell_pipe01.sh
> diff --git a/runtest/syscalls b/runtest/syscalls
> index b9dd9fec6..f515d46aa 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1156,7 +1156,6 @@ removexattr01 removexattr01
>  removexattr02 removexattr02

>  rename01 rename01
> -rename01A symlink01 -T rename01
>  rename03 rename03
>  rename04 rename04
>  rename05 rename05
> @@ -1169,6 +1168,7 @@ rename11 rename11
>  rename12 rename12
>  rename13 rename13
>  rename14 rename14
> +rename15 rename15

>  #renameat test cases
nit: maybe while at it remove this useless comment above?

>  renameat01 renameat01
> diff --git a/testcases/kernel/syscalls/rename/.gitignore b/testcases/kernel/syscalls/rename/.gitignore
> index f95cf7d21..d17b80f09 100644
> --- a/testcases/kernel/syscalls/rename/.gitignore
> +++ b/testcases/kernel/syscalls/rename/.gitignore
> @@ -11,3 +11,4 @@
>  /rename12
>  /rename13
>  /rename14
> +/rename15
> diff --git a/testcases/kernel/syscalls/rename/rename15.c b/testcases/kernel/syscalls/rename/rename15.c
> new file mode 100644
> index 000000000..d410758a2
> --- /dev/null
> +++ b/testcases/kernel/syscalls/rename/rename15.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
> + *    Author: David Fenner
> + *    Copilot: Jon Hendrickson
> + * Copyright (C) 2024 Andrea Cervesato andrea.cervesato@suse.com
very nit:
*  Authors: David Fenner, Jon Hendrickson
 * Copyright (C) 2024 Andrea Cervesato <andrea.cervesato@suse.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test verifies that rename() is working correctly on symlink()
> + * generated files.
> + */
> +
> +#include <stdlib.h>
> +#include "tst_test.h"
> +
> +#define OLDNAME "msymlink0"
> +#define NEWNAME "asymlink0"
> +
> +static char *tmpdir;
> +
> +static void test_existing(void)
> +{
> +	tst_res(TINFO, "Test rename() on symlink pointing to an existent path");
> +
> +	struct stat oldsym_stat;
> +	struct stat newsym_stat;
> +
> +	SAFE_SYMLINK(tmpdir, OLDNAME);
> +	SAFE_STAT(OLDNAME, &oldsym_stat);
> +
> +	SAFE_RENAME(OLDNAME, NEWNAME);
> +	SAFE_STAT(NEWNAME, &newsym_stat);
> +
> +	TST_EXP_EQ_LI(oldsym_stat.st_ino, newsym_stat.st_ino);
> +	TST_EXP_EQ_LI(oldsym_stat.st_dev, newsym_stat.st_dev);
> +
> +	SAFE_UNLINK(NEWNAME);
> +}
> +
> +static void test_non_existing(void)
> +{
> +	tst_res(TINFO, "Test rename() on symlink pointing to a non-existent path");
> +
> +	struct stat path_stat;
> +
> +	SAFE_SYMLINK("this_path_doesnt_exist", OLDNAME);
> +	TST_EXP_FAIL(stat(OLDNAME, &path_stat), ENOENT);
> +
> +	SAFE_RENAME(OLDNAME, NEWNAME);
> +	TST_EXP_FAIL(stat(NEWNAME, &path_stat), ENOENT);
> +
> +	SAFE_UNLINK(NEWNAME);
> +}
> +
> +static void test_creat(void)
> +{
> +	tst_res(TINFO, "Test rename() on symlink pointing to a path created lately");
> +
> +	char *objpath = "object";
nit: why not above:
#define OBJPATH "object"
and use this definition.
Not really important, but definitions are more common and you would be
consistent.

> +	struct stat path_stat;
	int fd;
(see below)

> +
> +	SAFE_SYMLINK(objpath, OLDNAME);
> +	TST_EXP_FAIL(stat(OLDNAME, &path_stat), ENOENT);
> +
> +	tst_res(TINFO, "Create object file");
> +
> +	SAFE_CREAT(objpath, 0700);
> +	SAFE_RENAME(OLDNAME, NEWNAME);
> +	TST_EXP_PASS(stat(NEWNAME, &path_stat));
> +
> +	SAFE_UNLINK(objpath);
> +	SAFE_UNLINK(NEWNAME);

You need to close(), because with too high -i it fails:

./rename15 -i1200
rename15.c:25: TINFO: Test rename() on symlink pointing to an existent path
rename15.c:36: TPASS: oldsym_stat.st_ino == newsym_stat.st_ino (92519)
rename15.c:37: TPASS: oldsym_stat.st_dev == newsym_stat.st_dev (40)
rename15.c:59: TINFO: Test rename() on symlink pointing to a path created lately
rename15.c:65: TPASS: stat(OLDNAME, &path_stat) : ENOENT (2)
rename15.c:67: TINFO: Create object file
rename15.c:69: TBROK: creat(object,0700) failed: EMFILE (24)

	fd = SAFE_CREAT(OBJPATH, 0700);
	if (fd >= 0)
		SAFE_CLOSE(fd);

> +}
> +
> +static void run(void)
> +{
> +	test_existing();
> +	test_creat();
> +	test_non_existing();
> +}
> +
> +static void setup(void)
> +{
> +	tmpdir = tst_get_tmpdir();
This will need to be replaced to tst_tmpdir_path();

> +}
> +
> +static void cleanup(void)
> +{
> +	free(tmpdir);
> +}
And you now don't need a cleanup function now (done by library).

With fixed this and -i and ideally with the other diff below (but it is up to
you) you may add:

Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.needs_tmpdir = 1,
I also wonder why we don't run this on all filesystems as we do on most of other
rename tests.
> +};

Kind regards,
Petr

diff --git testcases/kernel/syscalls/rename/rename15.c testcases/kernel/syscalls/rename/rename15.c
index d410758a20..8bda162e56 100644
--- testcases/kernel/syscalls/rename/rename15.c
+++ testcases/kernel/syscalls/rename/rename15.c
@@ -1,9 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
- *    Author: David Fenner
- *    Copilot: Jon Hendrickson
- * Copyright (C) 2024 Andrea Cervesato andrea.cervesato@suse.com
+ * Authors: David Fenner, Jon Hendrickson
+ * Copyright (C) 2024 Andrea Cervesato <andrea.cervesato@suse.com>
  */
 
 /*\
@@ -16,6 +15,7 @@
 #include <stdlib.h>
 #include "tst_test.h"
 
+#define OBJPATH "object"
 #define OLDNAME "msymlink0"
 #define NEWNAME "asymlink0"
 
@@ -59,19 +59,22 @@ static void test_creat(void)
 {
 	tst_res(TINFO, "Test rename() on symlink pointing to a path created lately");
 
-	char *objpath = "object";
 	struct stat path_stat;
+	int fd;
 
-	SAFE_SYMLINK(objpath, OLDNAME);
+	SAFE_SYMLINK(OBJPATH, OLDNAME);
 	TST_EXP_FAIL(stat(OLDNAME, &path_stat), ENOENT);
 
 	tst_res(TINFO, "Create object file");
 
-	SAFE_CREAT(objpath, 0700);
+	fd = SAFE_CREAT(OBJPATH, 0700);
+	if (fd >= 0)
+		SAFE_CLOSE(fd);
+
 	SAFE_RENAME(OLDNAME, NEWNAME);
 	TST_EXP_PASS(stat(NEWNAME, &path_stat));
 
-	SAFE_UNLINK(objpath);
+	SAFE_UNLINK(OBJPATH);
 	SAFE_UNLINK(NEWNAME);
 }
 
@@ -84,17 +87,11 @@ static void run(void)
 
 static void setup(void)
 {
-	tmpdir = tst_get_tmpdir();
-}
-
-static void cleanup(void)
-{
-	free(tmpdir);
+	tmpdir = tst_tmpdir_path();
 }
 
 static struct tst_test test = {
 	.setup = setup,
-	.cleanup = cleanup,
 	.test_all = run,
 	.needs_tmpdir = 1,
 };

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