Re: [LTP] [PATCH] Fix unlink09 test

[Petr Vorel <pvorel@suse.cz>]

Hi Andrea,

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

> This patch will fix unlink09 test by checking for filesystems which
> are not supporting inode attributes.

> Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS)
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> This will fix the 2cf78f47a6 and resolve issues on filesystems
> which are not supporting inode attributes.
> ---
>  testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 13 deletions(-)

> diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c
> index cc4b4a07e..ed6f0adc3 100644
> --- a/testcases/kernel/syscalls/unlink/unlink09.c
> +++ b/testcases/kernel/syscalls/unlink/unlink09.c
> @@ -11,6 +11,8 @@
>   *
>   * - EPERM when target file is marked as immutable or append-only
>   * - EROFS when target file is on a read-only filesystem.
> + *
> + * Test won't be executed if inode attributes are not supported.
While this is good, wouldn't be better to use approach from Avinesh to add
O_RDWR (or whatever flag) to get test supported everywhere instead of skip?

https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/

>   */

>  #include <sys/ioctl.h>
> @@ -22,8 +24,8 @@
>  #define DIR_EROFS "erofs"
>  #define TEST_EROFS "erofs/test_erofs"

> -static int fd_immutable;
> -static int fd_append_only;
> +static int fd_immutable = -1;
> +static int fd_append_only = -1;

>  static struct test_case_t {
>  	char *filename;
> @@ -43,12 +45,18 @@ static void setup(void)
>  {
>  	int attr;

> -	fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600);
> -	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> +	fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600);
> +	TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr));
> +
> +	if (TST_RET == -1 && TST_ERR == ENOTTY) {
> +		SAFE_CLOSE(fd_immutable);
> +		tst_brk(TCONF | TTERRNO, "Inode attributes not supported");
> +	}
> +
>  	attr |= FS_IMMUTABLE_FL;
>  	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);

> -	fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600);
> +	fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600);
>  	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
>  	attr |= FS_APPEND_FL;
>  	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> @@ -58,15 +66,19 @@ static void cleanup(void)
>  {
>  	int attr;

> -	SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> -	attr &= ~FS_IMMUTABLE_FL;
> -	SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> -	SAFE_CLOSE(fd_immutable);
> +	if (fd_immutable != -1) {
I thought we could return when fd_immutable == -1:

	if (fd_immutable != -1)
		return;

But obviously this is can be also result of the first test (not only by the
setup), thus you're correct.

BTW verify_unlink() could be made simpler to return on if (TST_RET).

> +		SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr);
> +		attr &= ~FS_IMMUTABLE_FL;
> +		SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr);
> +		SAFE_CLOSE(fd_immutable);
> +	}

> -	SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> -	attr &= ~FS_APPEND_FL;
> -	SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> -	SAFE_CLOSE(fd_append_only);
> +	if (fd_append_only != -1) {
> +		SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr);
> +		attr &= ~FS_APPEND_FL;
> +		SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr);
> +		SAFE_CLOSE(fd_append_only);
> +	}
Am I mistaken that this check should have been added before?

>  }

>  static void verify_unlink(unsigned int i)

> ---
> base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f
I see useful b4 feature :).

> change-id: 20240604-unlink09-dc4802f872f9
But I wonder what is this for (what tool use change-id).

Kind regards,
Petr

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