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

[Petr Vorel <pvorel@suse.cz>]

> From: Andrea Cervesato <andrea.cervesato@suse.com>

> This test has been extracted from symlink01 and it verifies that
> rename() is working correctly on symlink() generated files, renaming
> symbolic link and checking if stat() information are preserved.

The original test does 2 things:
1) Create symlink to *non-existing* file, then rename symlink:
symlink("object", "symbolic")           = 0
rename("symbolic", "asymbolic")         = 0

and check struct stat content.

2) Create symlink to *non-existing* file, but then creates the file, so that
it's symlink to an existing file (I would personally first call creat()).

symlink("object", "symbolic")           = 0
creat("object", 0700)                   = 3
rename("symbolic", "asymbolic")         = 0

Therefore you are testing only 2), maybe it'd be worth to test both cases.
I suppose there is no point to test on more file types (directory, char device,
...).

Also, it performs lstat() and then it checks against S_IFLNK,
but we can probably be ok just with safe_symlink(). I'm repeating myself,
because you send these symlink01.c related tests as separate patches, but IMHO
they are related (if v2 is needed it might make sense to sent all you sent in a
patchset, not separate).


> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  runtest/smoketest                           |  2 +-
>  runtest/syscalls                            |  2 +-
>  testcases/kernel/syscalls/rename/.gitignore |  1 +
>  testcases/kernel/syscalls/rename/rename15.c | 41 +++++++++++++++++++++
>  4 files changed, 44 insertions(+), 2 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/rename/rename15.c

> diff --git a/runtest/smoketest b/runtest/smoketest
> index 83eebfe7b..19fa257d6 100644
> --- a/runtest/smoketest
> +++ 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
>  splice02 splice02 -s 20
>  df01_sh df01.sh
>  shell_test01 echo "SUCCESS" | shell_pipe01.sh
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 6e2407879..ae058153b 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1158,7 +1158,6 @@ removexattr01 removexattr01
>  removexattr02 removexattr02

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

>  #renameat test cases
>  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..ae7f330b6
> --- /dev/null
> +++ b/testcases/kernel/syscalls/rename/rename15.c
> @@ -0,0 +1,41 @@
> +// 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
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This test verifies that rename() is working correctly on symlink()
> + * generated files, renaming symbolic link and checking if stat() information
> + * are preserved.
> + */
> +

very nit: maybe it'd be safer to actually use headers which are needed.
One day, if we rewrite library, we will have to keep all the headers in
tst_test.h, even not needed in the library, because tests starts to depend on
it.

#include <stdio.h>
#include <sys/stat.h>
#include <unistd.h>

> +#include "tst_test.h"
> +
> +static void run(void)
> +{
> +	char *oldname = "my_symlink0";
> +	char *newname = "asymlink";

These aren't modified, how about use:

#define OLDNAME "my_symlink0"
#define NEWNAME "asymlink"

> +	struct stat oldsym_stat;
> +	struct stat newsym_stat;
> +
> +	SAFE_SYMLINK(tst_get_tmpdir(), oldname);
Again, this looks to be a memory leak.

It's noted in the headers, but IMHO it should be added to the docs:
https://github.com/linux-test-project/ltp/wiki/C-Test-API
https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist
and/or even write sparse check.

include/tst_test.h
/*
 * Returns path to the test temporary directory in a newly allocated buffer.
 */
char *tst_get_tmpdir(void);

include/old/old_tmpdir.h
/* tst_get_tmpdir()
 *
 * Return a copy of the test temp directory as seen by LTP. This is for
 * path-oriented tests like chroot, etc, that may munge the path a bit.
 *
 * FREE VARIABLE AFTER USE IF IT IS REUSED!
 */
char *tst_get_tmpdir(void);

Alternatives would be: 1) use "." (relative path instead of absolute,
2) call it in the setup function.

> +	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 struct tst_test test = {
> +	.test_all = run,
> +	.needs_tmpdir = 1,
> +};

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