* [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag
@ 2023-05-22 9:44 Yang Xu
2023-05-29 17:50 ` Petr Vorel
0 siblings, 1 reply; 5+ messages in thread
From: Yang Xu @ 2023-05-22 9:44 UTC (permalink / raw)
To: ltp
This flag was introduced since kernel 5.10 and was used to indicates
whether the path or fd refers to the root of a mount or not.
Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
include/lapi/stat.h | 4 +
runtest/syscalls | 1 +
testcases/kernel/syscalls/statx/.gitignore | 1 +
testcases/kernel/syscalls/statx/statx12.c | 101 +++++++++++++++++++++
4 files changed, 107 insertions(+)
create mode 100644 testcases/kernel/syscalls/statx/statx12.c
diff --git a/include/lapi/stat.h b/include/lapi/stat.h
index c7e6fdac0..3606c9eb0 100644
--- a/include/lapi/stat.h
+++ b/include/lapi/stat.h
@@ -221,6 +221,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
# define STATX_ATTR_AUTOMOUNT 0x00001000
#endif
+#ifndef STATX_ATTR_MOUNT_ROOT
+# define STATX_ATTR_MOUNT_ROOT 0x00002000
+#endif
+
#ifndef STATX_ATTR_VERITY
# define STATX_ATTR_VERITY 0x00100000
#endif
diff --git a/runtest/syscalls b/runtest/syscalls
index e5ad2c2f9..0c1993421 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1767,6 +1767,7 @@ statx08 statx08
statx09 statx09
statx10 statx10
statx11 statx11
+statx12 statx12
membarrier01 membarrier01
diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
index 48ac4078b..f6a423eed 100644
--- a/testcases/kernel/syscalls/statx/.gitignore
+++ b/testcases/kernel/syscalls/statx/.gitignore
@@ -9,3 +9,4 @@
/statx09
/statx10
/statx11
+/statx12
diff --git a/testcases/kernel/syscalls/statx/statx12.c b/testcases/kernel/syscalls/statx/statx12.c
new file mode 100644
index 000000000..ae6bbb1e2
--- /dev/null
+++ b/testcases/kernel/syscalls/statx/statx12.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
+ */
+
+/*\
+ * [Description]
+ *
+ * It is a basic test for STATX_ATTR_MOUNT_ROOT flag.
+ *
+ * This flag indicates whether the path or fd refers to the root of a mount
+ * or not.
+ *
+ * Minimum Linux version required is v5.10.
+ */
+
+#define _GNU_SOURCE
+#include <sys/types.h>
+#include <sys/mount.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include "tst_test.h"
+#include "lapi/stat.h"
+
+#define MNTPOINT "mntpoint"
+#define TESTFILE MNTPOINT"/testfile"
+
+static int dir_fd = -1, file_fd = -1;
+
+static struct tcase {
+ const char *path;
+ int mnt_root;
+ int flag;
+ int *fd;
+} tcases[] = {
+ {MNTPOINT, 1, 1, &dir_fd},
+ {MNTPOINT, 1, 0, &dir_fd},
+ {TESTFILE, 0, 1, &file_fd},
+ {TESTFILE, 0, 0, &file_fd}
+};
+
+static void verify_statx(unsigned int n)
+{
+ struct tcase *tc = &tcases[n];
+ struct statx buf;
+
+ if (tc->flag) {
+ tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
+ tc->path);
+
+ TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
+ "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
+ } else {
+ tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
+ tc->path);
+ TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
+ "statx(%d, "", 0, 0, &buf)", *tc->fd);
+ }
+
+ if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
+ tst_res(TCONF, "Filesystem does not support STATX_ATTR_MOUNT_ROOT");
+ return;
+ }
+
+ if (buf.stx_attributes & STATX_ATTR_MOUNT_ROOT) {
+ tst_res(tc->mnt_root ? TPASS : TFAIL,
+ "STATX_ATTR_MOUNT_ROOT flag is set");
+ } else {
+ tst_res(tc->mnt_root ? TFAIL : TPASS,
+ "STATX_ATTR_MOUNT_ROOT flag is not set");
+ }
+}
+
+static void setup(void)
+{
+ SAFE_CREAT(TESTFILE, 0755);
+ dir_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
+ file_fd = SAFE_OPEN(TESTFILE, O_RDWR);
+}
+
+static void cleanup(void)
+{
+ if (dir_fd > -1)
+ SAFE_CLOSE(dir_fd);
+ if (file_fd > -1)
+ SAFE_CLOSE(file_fd);
+}
+
+static struct tst_test test = {
+ .test = verify_statx,
+ .setup = setup,
+ .cleanup = cleanup,
+ .mntpoint = MNTPOINT,
+ .mount_device = 1,
+ .all_filesystems = 1,
+ .needs_root = 1,
+ .tcnt = ARRAY_SIZE(tcases)
+};
--
2.39.1
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag
2023-05-22 9:44 [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag Yang Xu
@ 2023-05-29 17:50 ` Petr Vorel
2023-05-30 5:52 ` Yang Xu (Fujitsu)
2023-06-25 14:47 ` Yang Xu (Fujitsu)
0 siblings, 2 replies; 5+ messages in thread
From: Petr Vorel @ 2023-05-29 17:50 UTC (permalink / raw)
To: Yang Xu; +Cc: ltp
Hi Xu,
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Tested-by: Petr Vorel <pvorel@suse.cz>
I tested it on recent kernel 6.3.1 and very old kernel 3.16.0.
Works as expected.
> This flag was introduced since kernel 5.10 and was used to indicates
80340fe3605c ("statx: add mount_root") was released in v5.8. Unless you refer to
something different, please update it here.
> whether the path or fd refers to the root of a mount or not.
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
> include/lapi/stat.h | 4 +
> runtest/syscalls | 1 +
> testcases/kernel/syscalls/statx/.gitignore | 1 +
> testcases/kernel/syscalls/statx/statx12.c | 101 +++++++++++++++++++++
> 4 files changed, 107 insertions(+)
> create mode 100644 testcases/kernel/syscalls/statx/statx12.c
> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
> index c7e6fdac0..3606c9eb0 100644
> --- a/include/lapi/stat.h
> +++ b/include/lapi/stat.h
> @@ -221,6 +221,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
> # define STATX_ATTR_AUTOMOUNT 0x00001000
> #endif
> +#ifndef STATX_ATTR_MOUNT_ROOT
> +# define STATX_ATTR_MOUNT_ROOT 0x00002000
> +#endif
> +
> #ifndef STATX_ATTR_VERITY
> # define STATX_ATTR_VERITY 0x00100000
> #endif
> diff --git a/runtest/syscalls b/runtest/syscalls
> index e5ad2c2f9..0c1993421 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1767,6 +1767,7 @@ statx08 statx08
> statx09 statx09
> statx10 statx10
> statx11 statx11
> +statx12 statx12
> membarrier01 membarrier01
> diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
> index 48ac4078b..f6a423eed 100644
> --- a/testcases/kernel/syscalls/statx/.gitignore
> +++ b/testcases/kernel/syscalls/statx/.gitignore
> @@ -9,3 +9,4 @@
> /statx09
> /statx10
> /statx11
> +/statx12
> diff --git a/testcases/kernel/syscalls/statx/statx12.c b/testcases/kernel/syscalls/statx/statx12.c
> new file mode 100644
> index 000000000..ae6bbb1e2
> --- /dev/null
> +++ b/testcases/kernel/syscalls/statx/statx12.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * It is a basic test for STATX_ATTR_MOUNT_ROOT flag.
> + *
> + * This flag indicates whether the path or fd refers to the root of a mount
> + * or not.
> + *
> + * Minimum Linux version required is v5.10.
And here as well.
> + */
> +
> +#define _GNU_SOURCE
> +#include <sys/types.h>
> +#include <sys/mount.h>
Do we need these two for anything?
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include "tst_test.h"
> +#include "lapi/stat.h"
> +
> +#define MNTPOINT "mntpoint"
> +#define TESTFILE MNTPOINT"/testfile"
> +
> +static int dir_fd = -1, file_fd = -1;
> +
> +static struct tcase {
> + const char *path;
> + int mnt_root;
> + int flag;
Since you're already using <stdbool.h>, mnt_root and flag could be bool.
> + int *fd;
> +} tcases[] = {
> + {MNTPOINT, 1, 1, &dir_fd},
> + {MNTPOINT, 1, 0, &dir_fd},
> + {TESTFILE, 0, 1, &file_fd},
> + {TESTFILE, 0, 0, &file_fd}
I'd even consider replacing int flag in struct with counted from n:
static struct tcase {
const char *path;
bool mnt_root;
int *fd;
} tcases[] = {
{MNTPOINT, 1, &dir_fd},
{TESTFILE, 0, &file_fd}
};
static void verify_statx(unsigned int n)
{
struct tcase *tc = &tcases[n/2];
struct statx buf;
bool flag = n % 2;
if (flag) {
tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
tc->path);
TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
} else {
tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
tc->path);
TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
}
...
static struct tst_test test = {
...
.tcnt = 2* ARRAY_SIZE(tcases)
> +};
> +
> +static void verify_statx(unsigned int n)
> +{
> + struct tcase *tc = &tcases[n];
> + struct statx buf;
> +
> + if (tc->flag) {
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> + tc->path);
> +
> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
> + "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
nit: I wonder if we need to duplicate the call in string, just to get tc->path,
which we have printed in TINFO above. Wouldn't be enough just:
TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
> + } else {
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> + tc->path);
> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
> + "statx(%d, "", 0, 0, &buf)", *tc->fd);
Well, here you probably meant
TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
"statx(%d, \"\", 0, 0, &buf)", *tc->fd);
checkpatch.pl (via make check-statx12) warns about it (slightly cryptic):
statx12.c:60: WARNING: Consecutive strings are generally better as a single string
TODO: this is the second thing which should be fixed before merge.
But again, I'd go just for:
TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
> + }
> +
> + if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
> + tst_res(TCONF, "Filesystem does not support STATX_ATTR_MOUNT_ROOT");
> + return;
> + }
> +
> + if (buf.stx_attributes & STATX_ATTR_MOUNT_ROOT) {
> + tst_res(tc->mnt_root ? TPASS : TFAIL,
> + "STATX_ATTR_MOUNT_ROOT flag is set");
> + } else {
> + tst_res(tc->mnt_root ? TFAIL : TPASS,
> + "STATX_ATTR_MOUNT_ROOT flag is not set");
> + }
> +}
> +
> +static void setup(void)
> +{
> + SAFE_CREAT(TESTFILE, 0755);
> + dir_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
> + file_fd = SAFE_OPEN(TESTFILE, O_RDWR);
> +}
> +
> +static void cleanup(void)
> +{
> + if (dir_fd > -1)
> + SAFE_CLOSE(dir_fd);
nit: Here could be empty line (readability).
> + if (file_fd > -1)
> + SAFE_CLOSE(file_fd);
> +}
> +
> +static struct tst_test test = {
> + .test = verify_statx,
> + .setup = setup,
> + .cleanup = cleanup,
> + .mntpoint = MNTPOINT,
> + .mount_device = 1,
> + .all_filesystems = 1,
> + .needs_root = 1,
> + .tcnt = ARRAY_SIZE(tcases)
> +};
All my suggestion (ok to ignore).
Kind regards,
Petr
diff --git testcases/kernel/syscalls/statx/statx12.c testcases/kernel/syscalls/statx/statx12.c
index 40367c8b1..6b76b6e49 100644
--- testcases/kernel/syscalls/statx/statx12.c
+++ testcases/kernel/syscalls/statx/statx12.c
@@ -12,12 +12,10 @@
* This flag indicates whether the path or fd refers to the root of a mount
* or not.
*
- * Minimum Linux version required is v5.10.
+ * Minimum Linux version required is v5.8.
*/
#define _GNU_SOURCE
-#include <sys/types.h>
-#include <sys/mount.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdbool.h>
@@ -32,32 +30,27 @@ static int dir_fd = -1, file_fd = -1;
static struct tcase {
const char *path;
- int mnt_root;
- int flag;
+ bool mnt_root;
int *fd;
} tcases[] = {
- {MNTPOINT, 1, 1, &dir_fd},
- {MNTPOINT, 1, 0, &dir_fd},
- {TESTFILE, 0, 1, &file_fd},
- {TESTFILE, 0, 0, &file_fd}
+ {MNTPOINT, 1, &dir_fd},
+ {TESTFILE, 0, &file_fd}
};
static void verify_statx(unsigned int n)
{
- struct tcase *tc = &tcases[n];
+ struct tcase *tc = &tcases[n/2];
struct statx buf;
+ bool flag = n % 2;
- if (tc->flag) {
- tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
+ if (flag) {
+ tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
tc->path);
-
- TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
- "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
+ TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
} else {
- tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
+ tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
tc->path);
- TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
- "statx(%d, "", 0, 0, &buf)", *tc->fd);
+ TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
}
if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
@@ -85,6 +78,7 @@ static void cleanup(void)
{
if (dir_fd > -1)
SAFE_CLOSE(dir_fd);
+
if (file_fd > -1)
SAFE_CLOSE(file_fd);
}
@@ -97,5 +91,5 @@ static struct tst_test test = {
.mount_device = 1,
.all_filesystems = 1,
.needs_root = 1,
- .tcnt = ARRAY_SIZE(tcases)
+ .tcnt = 2* ARRAY_SIZE(tcases)
};
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag
2023-05-29 17:50 ` Petr Vorel
@ 2023-05-30 5:52 ` Yang Xu (Fujitsu)
2023-06-02 3:13 ` Yang Xu (Fujitsu)
2023-06-25 14:47 ` Yang Xu (Fujitsu)
1 sibling, 1 reply; 5+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-05-30 5:52 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp@lists.linux.it
Hi Petr
> Hi Xu,
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Tested-by: Petr Vorel <pvorel@suse.cz>
>
> I tested it on recent kernel 6.3.1 and very old kernel 3.16.0.
> Works as expected.
>
>
>> This flag was introduced since kernel 5.10 and was used to indicates
>
> 80340fe3605c ("statx: add mount_root") was released in v5.8. Unless you refer to
> something different, please update it here.
# git tag --contains 80340fe3
Oh, sorry, I used the following command to search and miss v5.8
v5.10
v5.10-rc1
v5.10-rc2
v5.10-rc3
v5.10-rc4
v5.10-rc5
v5.10-rc6
v5.10-rc7
v5.11
v5.11-rc1
v5.11-rc2
v5.11-rc3
v5.11-rc4
v5.11-rc5
v5.11-rc6
v5.11-rc7
v5.12
v5.12-rc1
v5.12-rc1-dontuse
v5.12-rc2
v5.12-rc3
v5.12-rc4
v5.12-rc5
v5.12-rc6
v5.12-rc7
....
v5.8-rc1
>
>> whether the path or fd refers to the root of a mount or not.
>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>> include/lapi/stat.h | 4 +
>> runtest/syscalls | 1 +
>> testcases/kernel/syscalls/statx/.gitignore | 1 +
>> testcases/kernel/syscalls/statx/statx12.c | 101 +++++++++++++++++++++
>> 4 files changed, 107 insertions(+)
>> create mode 100644 testcases/kernel/syscalls/statx/statx12.c
>
>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>> index c7e6fdac0..3606c9eb0 100644
>> --- a/include/lapi/stat.h
>> +++ b/include/lapi/stat.h
>> @@ -221,6 +221,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>> # define STATX_ATTR_AUTOMOUNT 0x00001000
>> #endif
>
>> +#ifndef STATX_ATTR_MOUNT_ROOT
>> +# define STATX_ATTR_MOUNT_ROOT 0x00002000
>> +#endif
>> +
>> #ifndef STATX_ATTR_VERITY
>> # define STATX_ATTR_VERITY 0x00100000
>> #endif
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index e5ad2c2f9..0c1993421 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -1767,6 +1767,7 @@ statx08 statx08
>> statx09 statx09
>> statx10 statx10
>> statx11 statx11
>> +statx12 statx12
>
>> membarrier01 membarrier01
>
>> diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
>> index 48ac4078b..f6a423eed 100644
>> --- a/testcases/kernel/syscalls/statx/.gitignore
>> +++ b/testcases/kernel/syscalls/statx/.gitignore
>> @@ -9,3 +9,4 @@
>> /statx09
>> /statx10
>> /statx11
>> +/statx12
>> diff --git a/testcases/kernel/syscalls/statx/statx12.c b/testcases/kernel/syscalls/statx/statx12.c
>> new file mode 100644
>> index 000000000..ae6bbb1e2
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/statx/statx12.c
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * It is a basic test for STATX_ATTR_MOUNT_ROOT flag.
>> + *
>> + * This flag indicates whether the path or fd refers to the root of a mount
>> + * or not.
>> + *
>> + * Minimum Linux version required is v5.10.
> And here as well.
Yes.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <sys/types.h>
>> +#include <sys/mount.h>
> Do we need these two for anything?
I guess we can remove them.
>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdbool.h>
>> +#include <stdio.h>
>> +#include "tst_test.h"
>> +#include "lapi/stat.h"
>> +
>> +#define MNTPOINT "mntpoint"
>> +#define TESTFILE MNTPOINT"/testfile"
>> +
>> +static int dir_fd = -1, file_fd = -1;
>> +
>> +static struct tcase {
>> + const char *path;
>> + int mnt_root;
>> + int flag;
> Since you're already using <stdbool.h>, mnt_root and flag could be bool.
Yes.
>> + int *fd;
>> +} tcases[] = {
>> + {MNTPOINT, 1, 1, &dir_fd},
>> + {MNTPOINT, 1, 0, &dir_fd},
>> + {TESTFILE, 0, 1, &file_fd},
>> + {TESTFILE, 0, 0, &file_fd}
> I'd even consider replacing int flag in struct with counted from n:
>
> static struct tcase {
> const char *path;
> bool mnt_root;
> int *fd;
> } tcases[] = {
> {MNTPOINT, 1, &dir_fd},
> {TESTFILE, 0, &file_fd}
> };
>
> static void verify_statx(unsigned int n)
> {
> struct tcase *tc = &tcases[n/2];
> struct statx buf;
> bool flag = n % 2;
>
> if (flag) {
> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> tc->path);
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
> } else {
> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> tc->path);
>
> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
> }
> ...
>
> static struct tst_test test = {
> ...
> .tcnt = 2* ARRAY_SIZE(tcases)
>
>
>> +};
>> +
>> +static void verify_statx(unsigned int n)
>> +{
>> + struct tcase *tc = &tcases[n];
>> + struct statx buf;
>> +
>> + if (tc->flag) {
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>> + tc->path);
>> +
>> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
>> + "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
> nit: I wonder if we need to duplicate the call in string, just to get tc->path,
> which we have printed in TINFO above. Wouldn't be enough just:
> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
Sound reasonable.
>
>> + } else {
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>> + tc->path);
>> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
>> + "statx(%d, "", 0, 0, &buf)", *tc->fd);
> Well, here you probably meant
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
> "statx(%d, \"\", 0, 0, &buf)", *tc->fd);
> checkpatch.pl (via make check-statx12) warns about it (slightly cryptic):
> statx12.c:60: WARNING: Consecutive strings are generally better as a single string
> TODO: this is the second thing which should be fixed before merge.
Sound reasonable.
>
> But again, I'd go just for:
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
OK.
>> + }
>> +
>> + if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
>> + tst_res(TCONF, "Filesystem does not support STATX_ATTR_MOUNT_ROOT");
>> + return;
>> + }
>> +
>> + if (buf.stx_attributes & STATX_ATTR_MOUNT_ROOT) {
>> + tst_res(tc->mnt_root ? TPASS : TFAIL,
>> + "STATX_ATTR_MOUNT_ROOT flag is set");
>> + } else {
>> + tst_res(tc->mnt_root ? TFAIL : TPASS,
>> + "STATX_ATTR_MOUNT_ROOT flag is not set");
>> + }
>> +}
>> +
>> +static void setup(void)
>> +{
>> + SAFE_CREAT(TESTFILE, 0755);
>> + dir_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
>> + file_fd = SAFE_OPEN(TESTFILE, O_RDWR);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + if (dir_fd > -1)
>> + SAFE_CLOSE(dir_fd);
> nit: Here could be empty line (readability).
>> + if (file_fd > -1)
>> + SAFE_CLOSE(file_fd);
>> +}
>> +
>> +static struct tst_test test = {
>> + .test = verify_statx,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .mntpoint = MNTPOINT,
>> + .mount_device = 1,
>> + .all_filesystems = 1,
>> + .needs_root = 1,
>> + .tcnt = ARRAY_SIZE(tcases)
>> +};
>
> All my suggestion (ok to ignore).
I agree with your all suggestion.
Best Regards
Yang Xu
>
> Kind regards,
> Petr
>
> diff --git testcases/kernel/syscalls/statx/statx12.c testcases/kernel/syscalls/statx/statx12.c
> index 40367c8b1..6b76b6e49 100644
> --- testcases/kernel/syscalls/statx/statx12.c
> +++ testcases/kernel/syscalls/statx/statx12.c
> @@ -12,12 +12,10 @@
> * This flag indicates whether the path or fd refers to the root of a mount
> * or not.
> *
> - * Minimum Linux version required is v5.10.
> + * Minimum Linux version required is v5.8.
> */
>
> #define _GNU_SOURCE
> -#include <sys/types.h>
> -#include <sys/mount.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stdbool.h>
> @@ -32,32 +30,27 @@ static int dir_fd = -1, file_fd = -1;
>
> static struct tcase {
> const char *path;
> - int mnt_root;
> - int flag;
> + bool mnt_root;
> int *fd;
> } tcases[] = {
> - {MNTPOINT, 1, 1, &dir_fd},
> - {MNTPOINT, 1, 0, &dir_fd},
> - {TESTFILE, 0, 1, &file_fd},
> - {TESTFILE, 0, 0, &file_fd}
> + {MNTPOINT, 1, &dir_fd},
> + {TESTFILE, 0, &file_fd}
> };
>
> static void verify_statx(unsigned int n)
> {
> - struct tcase *tc = &tcases[n];
> + struct tcase *tc = &tcases[n/2];
> struct statx buf;
> + bool flag = n % 2;
>
> - if (tc->flag) {
> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> + if (flag) {
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> tc->path);
> -
> - TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
> - "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
> } else {
> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> tc->path);
> - TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
> - "statx(%d, "", 0, 0, &buf)", *tc->fd);
> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
> }
>
> if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
> @@ -85,6 +78,7 @@ static void cleanup(void)
> {
> if (dir_fd > -1)
> SAFE_CLOSE(dir_fd);
> +
> if (file_fd > -1)
> SAFE_CLOSE(file_fd);
> }
> @@ -97,5 +91,5 @@ static struct tst_test test = {
> .mount_device = 1,
> .all_filesystems = 1,
> .needs_root = 1,
> - .tcnt = ARRAY_SIZE(tcases)
> + .tcnt = 2* ARRAY_SIZE(tcases)
> };
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag
2023-05-30 5:52 ` Yang Xu (Fujitsu)
@ 2023-06-02 3:13 ` Yang Xu (Fujitsu)
0 siblings, 0 replies; 5+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-06-02 3:13 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp@lists.linux.it
Hi Petr
Should I send a v2 patch for this, or you merged the modified patch
directly.
Best Regards
Yang Xu
> Hi Petr
>
>
>> Hi Xu,
>>
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> Tested-by: Petr Vorel <pvorel@suse.cz>
>>
>> I tested it on recent kernel 6.3.1 and very old kernel 3.16.0.
>> Works as expected.
>>
>>
>>> This flag was introduced since kernel 5.10 and was used to indicates
>>
>> 80340fe3605c ("statx: add mount_root") was released in v5.8. Unless you refer to
>> something different, please update it here.
> # git tag --contains 80340fe3
> Oh, sorry, I used the following command to search and miss v5.8
> v5.10
> v5.10-rc1
> v5.10-rc2
> v5.10-rc3
> v5.10-rc4
> v5.10-rc5
> v5.10-rc6
> v5.10-rc7
> v5.11
> v5.11-rc1
> v5.11-rc2
> v5.11-rc3
> v5.11-rc4
> v5.11-rc5
> v5.11-rc6
> v5.11-rc7
> v5.12
> v5.12-rc1
> v5.12-rc1-dontuse
> v5.12-rc2
> v5.12-rc3
> v5.12-rc4
> v5.12-rc5
> v5.12-rc6
> v5.12-rc7
> ....
> v5.8-rc1
>
>>
>>> whether the path or fd refers to the root of a mount or not.
>>
>>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>>> ---
>>> include/lapi/stat.h | 4 +
>>> runtest/syscalls | 1 +
>>> testcases/kernel/syscalls/statx/.gitignore | 1 +
>>> testcases/kernel/syscalls/statx/statx12.c | 101 +++++++++++++++++++++
>>> 4 files changed, 107 insertions(+)
>>> create mode 100644 testcases/kernel/syscalls/statx/statx12.c
>>
>>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>>> index c7e6fdac0..3606c9eb0 100644
>>> --- a/include/lapi/stat.h
>>> +++ b/include/lapi/stat.h
>>> @@ -221,6 +221,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>>> # define STATX_ATTR_AUTOMOUNT 0x00001000
>>> #endif
>>
>>> +#ifndef STATX_ATTR_MOUNT_ROOT
>>> +# define STATX_ATTR_MOUNT_ROOT 0x00002000
>>> +#endif
>>> +
>>> #ifndef STATX_ATTR_VERITY
>>> # define STATX_ATTR_VERITY 0x00100000
>>> #endif
>>> diff --git a/runtest/syscalls b/runtest/syscalls
>>> index e5ad2c2f9..0c1993421 100644
>>> --- a/runtest/syscalls
>>> +++ b/runtest/syscalls
>>> @@ -1767,6 +1767,7 @@ statx08 statx08
>>> statx09 statx09
>>> statx10 statx10
>>> statx11 statx11
>>> +statx12 statx12
>>
>>> membarrier01 membarrier01
>>
>>> diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
>>> index 48ac4078b..f6a423eed 100644
>>> --- a/testcases/kernel/syscalls/statx/.gitignore
>>> +++ b/testcases/kernel/syscalls/statx/.gitignore
>>> @@ -9,3 +9,4 @@
>>> /statx09
>>> /statx10
>>> /statx11
>>> +/statx12
>>> diff --git a/testcases/kernel/syscalls/statx/statx12.c b/testcases/kernel/syscalls/statx/statx12.c
>>> new file mode 100644
>>> index 000000000..ae6bbb1e2
>>> --- /dev/null
>>> +++ b/testcases/kernel/syscalls/statx/statx12.c
>>> @@ -0,0 +1,101 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +/*
>>> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
>>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>>> + */
>>> +
>>> +/*\
>>> + * [Description]
>>> + *
>>> + * It is a basic test for STATX_ATTR_MOUNT_ROOT flag.
>>> + *
>>> + * This flag indicates whether the path or fd refers to the root of a mount
>>> + * or not.
>>> + *
>>> + * Minimum Linux version required is v5.10.
>> And here as well.
>
> Yes.
>>> + */
>>> +
>>> +#define _GNU_SOURCE
>>> +#include <sys/types.h>
>>> +#include <sys/mount.h>
>> Do we need these two for anything?
> I guess we can remove them.
>>
>>> +#include <unistd.h>
>>> +#include <stdlib.h>
>>> +#include <stdbool.h>
>>> +#include <stdio.h>
>>> +#include "tst_test.h"
>>> +#include "lapi/stat.h"
>>> +
>>> +#define MNTPOINT "mntpoint"
>>> +#define TESTFILE MNTPOINT"/testfile"
>>> +
>>> +static int dir_fd = -1, file_fd = -1;
>>> +
>>> +static struct tcase {
>>> + const char *path;
>>> + int mnt_root;
>>> + int flag;
>> Since you're already using <stdbool.h>, mnt_root and flag could be bool.
>
> Yes.
>>> + int *fd;
>>> +} tcases[] = {
>>> + {MNTPOINT, 1, 1, &dir_fd},
>>> + {MNTPOINT, 1, 0, &dir_fd},
>>> + {TESTFILE, 0, 1, &file_fd},
>>> + {TESTFILE, 0, 0, &file_fd}
>> I'd even consider replacing int flag in struct with counted from n:
>>
>> static struct tcase {
>> const char *path;
>> bool mnt_root;
>> int *fd;
>> } tcases[] = {
>> {MNTPOINT, 1, &dir_fd},
>> {TESTFILE, 0, &file_fd}
>> };
>>
>> static void verify_statx(unsigned int n)
>> {
>> struct tcase *tc = &tcases[n/2];
>> struct statx buf;
>> bool flag = n % 2;
>>
>> if (flag) {
>> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>> tc->path);
>> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
>> } else {
>> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>> tc->path);
>>
>> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
>> }
>> ...
>>
>> static struct tst_test test = {
>> ...
>> .tcnt = 2* ARRAY_SIZE(tcases)
>>
>>
>>> +};
>>> +
>>> +static void verify_statx(unsigned int n)
>>> +{
>>> + struct tcase *tc = &tcases[n];
>>> + struct statx buf;
>>> +
>>> + if (tc->flag) {
>>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>>> + tc->path);
>>> +
>>> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
>>> + "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
>> nit: I wonder if we need to duplicate the call in string, just to get tc->path,
>> which we have printed in TINFO above. Wouldn't be enough just:
>> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
>
> Sound reasonable.
>>
>>> + } else {
>>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>>> + tc->path);
>>> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
>>> + "statx(%d, "", 0, 0, &buf)", *tc->fd);
>> Well, here you probably meant
>> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
>> "statx(%d, \"\", 0, 0, &buf)", *tc->fd);
>> checkpatch.pl (via make check-statx12) warns about it (slightly cryptic):
>> statx12.c:60: WARNING: Consecutive strings are generally better as a single string
>> TODO: this is the second thing which should be fixed before merge.
>
> Sound reasonable.
>>
>> But again, I'd go just for:
>> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
>
> OK.
>>> + }
>>> +
>>> + if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
>>> + tst_res(TCONF, "Filesystem does not support STATX_ATTR_MOUNT_ROOT");
>>> + return;
>>> + }
>>> +
>>> + if (buf.stx_attributes & STATX_ATTR_MOUNT_ROOT) {
>>> + tst_res(tc->mnt_root ? TPASS : TFAIL,
>>> + "STATX_ATTR_MOUNT_ROOT flag is set");
>>> + } else {
>>> + tst_res(tc->mnt_root ? TFAIL : TPASS,
>>> + "STATX_ATTR_MOUNT_ROOT flag is not set");
>>> + }
>>> +}
>>> +
>>> +static void setup(void)
>>> +{
>>> + SAFE_CREAT(TESTFILE, 0755);
>>> + dir_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
>>> + file_fd = SAFE_OPEN(TESTFILE, O_RDWR);
>>> +}
>>> +
>>> +static void cleanup(void)
>>> +{
>>> + if (dir_fd > -1)
>>> + SAFE_CLOSE(dir_fd);
>> nit: Here could be empty line (readability).
>>> + if (file_fd > -1)
>>> + SAFE_CLOSE(file_fd);
>>> +}
>>> +
>>> +static struct tst_test test = {
>>> + .test = verify_statx,
>>> + .setup = setup,
>>> + .cleanup = cleanup,
>>> + .mntpoint = MNTPOINT,
>>> + .mount_device = 1,
>>> + .all_filesystems = 1,
>>> + .needs_root = 1,
>>> + .tcnt = ARRAY_SIZE(tcases)
>>> +};
>>
>> All my suggestion (ok to ignore).
>
> I agree with your all suggestion.
>
> Best Regards
> Yang Xu
>>
>> Kind regards,
>> Petr
>>
>> diff --git testcases/kernel/syscalls/statx/statx12.c testcases/kernel/syscalls/statx/statx12.c
>> index 40367c8b1..6b76b6e49 100644
>> --- testcases/kernel/syscalls/statx/statx12.c
>> +++ testcases/kernel/syscalls/statx/statx12.c
>> @@ -12,12 +12,10 @@
>> * This flag indicates whether the path or fd refers to the root of a mount
>> * or not.
>> *
>> - * Minimum Linux version required is v5.10.
>> + * Minimum Linux version required is v5.8.
>> */
>>
>> #define _GNU_SOURCE
>> -#include <sys/types.h>
>> -#include <sys/mount.h>
>> #include <unistd.h>
>> #include <stdlib.h>
>> #include <stdbool.h>
>> @@ -32,32 +30,27 @@ static int dir_fd = -1, file_fd = -1;
>>
>> static struct tcase {
>> const char *path;
>> - int mnt_root;
>> - int flag;
>> + bool mnt_root;
>> int *fd;
>> } tcases[] = {
>> - {MNTPOINT, 1, 1, &dir_fd},
>> - {MNTPOINT, 1, 0, &dir_fd},
>> - {TESTFILE, 0, 1, &file_fd},
>> - {TESTFILE, 0, 0, &file_fd}
>> + {MNTPOINT, 1, &dir_fd},
>> + {TESTFILE, 0, &file_fd}
>> };
>>
>> static void verify_statx(unsigned int n)
>> {
>> - struct tcase *tc = &tcases[n];
>> + struct tcase *tc = &tcases[n/2];
>> struct statx buf;
>> + bool flag = n % 2;
>>
>> - if (tc->flag) {
>> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>> + if (flag) {
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>> tc->path);
>> -
>> - TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
>> - "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
>> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
>> } else {
>> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>> tc->path);
>> - TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
>> - "statx(%d, "", 0, 0, &buf)", *tc->fd);
>> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
>> }
>>
>> if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
>> @@ -85,6 +78,7 @@ static void cleanup(void)
>> {
>> if (dir_fd > -1)
>> SAFE_CLOSE(dir_fd);
>> +
>> if (file_fd > -1)
>> SAFE_CLOSE(file_fd);
>> }
>> @@ -97,5 +91,5 @@ static struct tst_test test = {
>> .mount_device = 1,
>> .all_filesystems = 1,
>> .needs_root = 1,
>> - .tcnt = ARRAY_SIZE(tcases)
>> + .tcnt = 2* ARRAY_SIZE(tcases)
>> };
>
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag
2023-05-29 17:50 ` Petr Vorel
2023-05-30 5:52 ` Yang Xu (Fujitsu)
@ 2023-06-25 14:47 ` Yang Xu (Fujitsu)
1 sibling, 0 replies; 5+ messages in thread
From: Yang Xu (Fujitsu) @ 2023-06-25 14:47 UTC (permalink / raw)
To: Petr Vorel; +Cc: ltp@lists.linux.it
Hi Petr
Thanks for you review, merged with your suggestion.
Best Regards
Yang Xu
> Hi Xu,
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Tested-by: Petr Vorel <pvorel@suse.cz>
>
> I tested it on recent kernel 6.3.1 and very old kernel 3.16.0.
> Works as expected.
>
>
>> This flag was introduced since kernel 5.10 and was used to indicates
>
> 80340fe3605c ("statx: add mount_root") was released in v5.8. Unless you refer to
> something different, please update it here.
>
>> whether the path or fd refers to the root of a mount or not.
>
>> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
>> ---
>> include/lapi/stat.h | 4 +
>> runtest/syscalls | 1 +
>> testcases/kernel/syscalls/statx/.gitignore | 1 +
>> testcases/kernel/syscalls/statx/statx12.c | 101 +++++++++++++++++++++
>> 4 files changed, 107 insertions(+)
>> create mode 100644 testcases/kernel/syscalls/statx/statx12.c
>
>> diff --git a/include/lapi/stat.h b/include/lapi/stat.h
>> index c7e6fdac0..3606c9eb0 100644
>> --- a/include/lapi/stat.h
>> +++ b/include/lapi/stat.h
>> @@ -221,6 +221,10 @@ static inline int statx(int dirfd, const char *pathname, unsigned int flags,
>> # define STATX_ATTR_AUTOMOUNT 0x00001000
>> #endif
>
>> +#ifndef STATX_ATTR_MOUNT_ROOT
>> +# define STATX_ATTR_MOUNT_ROOT 0x00002000
>> +#endif
>> +
>> #ifndef STATX_ATTR_VERITY
>> # define STATX_ATTR_VERITY 0x00100000
>> #endif
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index e5ad2c2f9..0c1993421 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -1767,6 +1767,7 @@ statx08 statx08
>> statx09 statx09
>> statx10 statx10
>> statx11 statx11
>> +statx12 statx12
>
>> membarrier01 membarrier01
>
>> diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore
>> index 48ac4078b..f6a423eed 100644
>> --- a/testcases/kernel/syscalls/statx/.gitignore
>> +++ b/testcases/kernel/syscalls/statx/.gitignore
>> @@ -9,3 +9,4 @@
>> /statx09
>> /statx10
>> /statx11
>> +/statx12
>> diff --git a/testcases/kernel/syscalls/statx/statx12.c b/testcases/kernel/syscalls/statx/statx12.c
>> new file mode 100644
>> index 000000000..ae6bbb1e2
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/statx/statx12.c
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2023 FUJITSU LIMITED. All rights reserved.
>> + * Author: Yang Xu <xuyang2018.jy@fujitsu.com>
>> + */
>> +
>> +/*\
>> + * [Description]
>> + *
>> + * It is a basic test for STATX_ATTR_MOUNT_ROOT flag.
>> + *
>> + * This flag indicates whether the path or fd refers to the root of a mount
>> + * or not.
>> + *
>> + * Minimum Linux version required is v5.10.
> And here as well.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <sys/types.h>
>> +#include <sys/mount.h>
> Do we need these two for anything?
>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdbool.h>
>> +#include <stdio.h>
>> +#include "tst_test.h"
>> +#include "lapi/stat.h"
>> +
>> +#define MNTPOINT "mntpoint"
>> +#define TESTFILE MNTPOINT"/testfile"
>> +
>> +static int dir_fd = -1, file_fd = -1;
>> +
>> +static struct tcase {
>> + const char *path;
>> + int mnt_root;
>> + int flag;
> Since you're already using <stdbool.h>, mnt_root and flag could be bool.
>> + int *fd;
>> +} tcases[] = {
>> + {MNTPOINT, 1, 1, &dir_fd},
>> + {MNTPOINT, 1, 0, &dir_fd},
>> + {TESTFILE, 0, 1, &file_fd},
>> + {TESTFILE, 0, 0, &file_fd}
> I'd even consider replacing int flag in struct with counted from n:
>
> static struct tcase {
> const char *path;
> bool mnt_root;
> int *fd;
> } tcases[] = {
> {MNTPOINT, 1, &dir_fd},
> {TESTFILE, 0, &file_fd}
> };
>
> static void verify_statx(unsigned int n)
> {
> struct tcase *tc = &tcases[n/2];
> struct statx buf;
> bool flag = n % 2;
>
> if (flag) {
> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> tc->path);
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
> } else {
> tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> tc->path);
>
> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
> }
> ...
>
> static struct tst_test test = {
> ...
> .tcnt = 2* ARRAY_SIZE(tcases)
>
>
>> +};
>> +
>> +static void verify_statx(unsigned int n)
>> +{
>> + struct tcase *tc = &tcases[n];
>> + struct statx buf;
>> +
>> + if (tc->flag) {
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
>> + tc->path);
>> +
>> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
>> + "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
> nit: I wonder if we need to duplicate the call in string, just to get tc->path,
> which we have printed in TINFO above. Wouldn't be enough just:
> TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
>
>> + } else {
>> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
>> + tc->path);
>> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
>> + "statx(%d, "", 0, 0, &buf)", *tc->fd);
> Well, here you probably meant
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
> "statx(%d, \"\", 0, 0, &buf)", *tc->fd);
> checkpatch.pl (via make check-statx12) warns about it (slightly cryptic):
> statx12.c:60: WARNING: Consecutive strings are generally better as a single string
> TODO: this is the second thing which should be fixed before merge.
>
> But again, I'd go just for:
> TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
>> + }
>> +
>> + if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
>> + tst_res(TCONF, "Filesystem does not support STATX_ATTR_MOUNT_ROOT");
>> + return;
>> + }
>> +
>> + if (buf.stx_attributes & STATX_ATTR_MOUNT_ROOT) {
>> + tst_res(tc->mnt_root ? TPASS : TFAIL,
>> + "STATX_ATTR_MOUNT_ROOT flag is set");
>> + } else {
>> + tst_res(tc->mnt_root ? TFAIL : TPASS,
>> + "STATX_ATTR_MOUNT_ROOT flag is not set");
>> + }
>> +}
>> +
>> +static void setup(void)
>> +{
>> + SAFE_CREAT(TESTFILE, 0755);
>> + dir_fd = SAFE_OPEN(MNTPOINT, O_DIRECTORY);
>> + file_fd = SAFE_OPEN(TESTFILE, O_RDWR);
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> + if (dir_fd > -1)
>> + SAFE_CLOSE(dir_fd);
> nit: Here could be empty line (readability).
>> + if (file_fd > -1)
>> + SAFE_CLOSE(file_fd);
>> +}
>> +
>> +static struct tst_test test = {
>> + .test = verify_statx,
>> + .setup = setup,
>> + .cleanup = cleanup,
>> + .mntpoint = MNTPOINT,
>> + .mount_device = 1,
>> + .all_filesystems = 1,
>> + .needs_root = 1,
>> + .tcnt = ARRAY_SIZE(tcases)
>> +};
>
> All my suggestion (ok to ignore).
>
> Kind regards,
> Petr
>
> diff --git testcases/kernel/syscalls/statx/statx12.c testcases/kernel/syscalls/statx/statx12.c
> index 40367c8b1..6b76b6e49 100644
> --- testcases/kernel/syscalls/statx/statx12.c
> +++ testcases/kernel/syscalls/statx/statx12.c
> @@ -12,12 +12,10 @@
> * This flag indicates whether the path or fd refers to the root of a mount
> * or not.
> *
> - * Minimum Linux version required is v5.10.
> + * Minimum Linux version required is v5.8.
> */
>
> #define _GNU_SOURCE
> -#include <sys/types.h>
> -#include <sys/mount.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <stdbool.h>
> @@ -32,32 +30,27 @@ static int dir_fd = -1, file_fd = -1;
>
> static struct tcase {
> const char *path;
> - int mnt_root;
> - int flag;
> + bool mnt_root;
> int *fd;
> } tcases[] = {
> - {MNTPOINT, 1, 1, &dir_fd},
> - {MNTPOINT, 1, 0, &dir_fd},
> - {TESTFILE, 0, 1, &file_fd},
> - {TESTFILE, 0, 0, &file_fd}
> + {MNTPOINT, 1, &dir_fd},
> + {TESTFILE, 0, &file_fd}
> };
>
> static void verify_statx(unsigned int n)
> {
> - struct tcase *tc = &tcases[n];
> + struct tcase *tc = &tcases[n/2];
> struct statx buf;
> + bool flag = n % 2;
>
> - if (tc->flag) {
> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> + if (flag) {
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> tc->path);
> -
> - TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf),
> - "statx(AT_FDCWD, %s, 0, 0, &buf)", tc->path);
> + TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf));
> } else {
> - tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by fd",
> + tst_res(TINFO, "Testing %s with STATX_ATTR_MOUNT_ROOT by path",
> tc->path);
> - TST_EXP_PASS_SILENT(statx(*tc->fd, "", AT_EMPTY_PATH, 0, &buf),
> - "statx(%d, "", 0, 0, &buf)", *tc->fd);
> + TST_EXP_PASS_SILENT(statx(AT_FDCWD, tc->path, 0, 0, &buf));
> }
>
> if (!(buf.stx_attributes_mask & STATX_ATTR_MOUNT_ROOT)) {
> @@ -85,6 +78,7 @@ static void cleanup(void)
> {
> if (dir_fd > -1)
> SAFE_CLOSE(dir_fd);
> +
> if (file_fd > -1)
> SAFE_CLOSE(file_fd);
> }
> @@ -97,5 +91,5 @@ static struct tst_test test = {
> .mount_device = 1,
> .all_filesystems = 1,
> .needs_root = 1,
> - .tcnt = ARRAY_SIZE(tcases)
> + .tcnt = 2* ARRAY_SIZE(tcases)
> };
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-25 14:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-22 9:44 [LTP] [PATCH] syscalls/statx12: Add basic test for STATX_ATTR_MOUNT_ROOT flag Yang Xu
2023-05-29 17:50 ` Petr Vorel
2023-05-30 5:52 ` Yang Xu (Fujitsu)
2023-06-02 3:13 ` Yang Xu (Fujitsu)
2023-06-25 14:47 ` Yang Xu (Fujitsu)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.