[LTP] [PATCH v1] fcntl40.c: Test fcntl using F_CREATED_QUERY

[Andrea Cervesato]

Hi!

A main comment about selftests ports is due to the way we should pay an 
extra attention to the code we are merging in LTP.
The LTP code base is different and we need to double check if the 
selftests make sense and we are correctly using available LTP API.
The documentation is also important and we should take a closer look to 
the description.

Some comments below.

On 12/28/24 12:44, Wei Gao via ltp wrote:
> Signed-off-by: Wei Gao <wegao@suse.com>
There's no commit description
> ---
>   include/lapi/fcntl.h                       |  8 ++++
>   runtest/syscalls                           |  2 +
>   testcases/kernel/syscalls/fcntl/.gitignore |  2 +
>   testcases/kernel/syscalls/fcntl/fcntl40.c  | 52 ++++++++++++++++++++++
>   4 files changed, 64 insertions(+)
>   create mode 100644 testcases/kernel/syscalls/fcntl/fcntl40.c
>
> diff --git a/include/lapi/fcntl.h b/include/lapi/fcntl.h
> index 761331798..7c0502488 100644
> --- a/include/lapi/fcntl.h
> +++ b/include/lapi/fcntl.h
> @@ -154,6 +154,14 @@
>   # define RENAME_WHITEOUT		(1 << 2)
>   #endif
>   
> +#ifndef F_LINUX_SPECIFIC_BASE
> +#define F_LINUX_SPECIFIC_BASE 1024
> +#endif
> +
> +#ifndef F_CREATED_QUERY
> +#define F_CREATED_QUERY (F_LINUX_SPECIFIC_BASE + 4)
> +#endif
> +
>   /* splice, vmsplice, tee */
>   
>   #ifndef SPLICE_F_NONBLOCK
> diff --git a/runtest/syscalls b/runtest/syscalls
> index ded035ee8..43e493eb1 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -363,6 +363,8 @@ fcntl38 fcntl38
>   fcntl38_64 fcntl38_64
>   fcntl39 fcntl39
>   fcntl39_64 fcntl39_64
> +fcntl40 fcntl40
> +fcntl40_64 fcntl40_64
>   
>   fdatasync01 fdatasync01
>   fdatasync02 fdatasync02
> diff --git a/testcases/kernel/syscalls/fcntl/.gitignore b/testcases/kernel/syscalls/fcntl/.gitignore
> index e60176973..e3486ee45 100644
> --- a/testcases/kernel/syscalls/fcntl/.gitignore
> +++ b/testcases/kernel/syscalls/fcntl/.gitignore
> @@ -72,3 +72,5 @@
>   /fcntl38_64
>   /fcntl39
>   /fcntl39_64
> +/fcntl40
> +/fcntl40_64
> diff --git a/testcases/kernel/syscalls/fcntl/fcntl40.c b/testcases/kernel/syscalls/fcntl/fcntl40.c
> new file mode 100644
> index 000000000..ef03becef
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fcntl/fcntl40.c
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 SUSE Wei Gao <wegao@suse.com>
> + */
> +
> +/*\
> + * [Description]
Now we don't need this anymore
> + *
> + * Basic test for fcntl using F_CREATED_QUERY.
"Verify if the fcntl() syscall is recognizing whether a file has been 
created or not via O_CREAT when O_CLOEXEC is also used."
> + *
> + * It is based on the following kernel commit:
> + * commit d0fe8920cbe42547798fd806f078eeaaba05df18
> + * Author: Christian Brauner brauner@kernel.org
> + * Date: Wed Jul 24 15:15:36 2024 +0200
To write if test is based on a kernel selftests is enough. The git tag 
should be pointed in the "struct tst_test" via ".tags".
> + */
> +
> +#include "lapi/fcntl.h"
> +#include "tst_test.h"
> +
> +static void verify_fcntl(void)
> +{
> +	for (int i = 0; i < 101; i++) {
This has been probably added by selftest devs due to make sure fcntl() 
is working correctly when multiple files are created.
I think in our case is not really useful, but we might have 2 options at 
this point:

1. we keep the loop, but we don't spam text messages in the stdout. 
Which means we probably need to remove TST_EXP_EQ_LI and to process 
multiple results at once
2. we remove the loop and we test it only on a single file

I'm more for the 2nd option.

> +		int fd;
> +		char path[PATH_MAX];
> +
> +		fd = SAFE_OPEN("/dev/null", O_RDONLY | O_CLOEXEC);
> +
> +		/* We didn't create "/dev/null". */
> +		TST_EXP_EQ_LI(fcntl(fd, F_CREATED_QUERY, 0), 0);
SAFE_FCNTL()
> +		close(fd);
SAFE_CLOSE()
> +
> +		sprintf(path, "aaaa_%d", i);
There's no need for this if we use a single file. We can just define a 
TEST_NAME macro on top and use that everywhere
> +		fd = SAFE_OPEN(path, O_CREAT | O_RDONLY | O_CLOEXEC, 0600);
> +
> +		/* We created "aaaa_%d". */
> +		TST_EXP_EQ_LI(fcntl(fd, F_CREATED_QUERY, 0), 1);
SAFE_FCNTL()
> +		close(fd);
SAFE_CLOSE()
> +
> +		fd = SAFE_OPEN(path, O_RDONLY | O_CLOEXEC);
> +
> +		/* We're opening it again, so no positive creation check. */
> +		TST_EXP_EQ_LI(fcntl(fd, F_CREATED_QUERY, 0), 0);
SAFE_FCNTL()
> +		close(fd);
SAFE_CLOSE()
> +		unlink(path);
SAFE_UNLINK()
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_fcntl,
> +	.needs_tmpdir = 1,
> +	.min_kver = "6.12",

Here we need to define .tags , adding the kernel commit that introduced 
the selftest.

> +};

Kind regards,
Andrea Cervesato