Skip to contentSkip to conversation

syscalls/statx: Add test for sync flags

PATCH Superseded Vaishnavi 3 participants 5 messages Dec 7, 2018 – Dec 13, 2018

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Vaishnavi

author
#0 lore raw

PATCH v4 1/1 syscalls/statx: Add test for sync flags

Testcase 1: AT_STATX_DONT_SYNC
With AT_STATX_DONT_SYNC server changes should not get sync with client.

Testcase 2: AT_STATX_FORCE_SYNC
With AT_STATX_FORCE_SYNC server changes should get sync with client.

Signed-off-by Kewal Ukunde <[email protected]>
Signed-off-by Vaishnavi.d <[email protected]>
Signed-off-by Tarun.T.U <[email protected]>
Reviewed-by Petr Vorel <[email protected]>
---
 runtest/syscalls                           |   1 +
 testcases/kernel/syscalls/statx/.gitignore |   1 +
 testcases/kernel/syscalls/statx/statx07.c  | 170 +++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 testcases/kernel/syscalls/statx/statx07.c
runtest/syscalls +1 -0
index ac1d2d2..957b999 100644
@@ -1524,5 +1524,6 @@ statx03 statx03
 statx04 statx04
 statx05 statx05
 statx06 statx06
+statx07 statx07
 
 membarrier01 membarrier01
testcases/kernel/syscalls/statx/.gitignore +1 -0
index c1cedda..2f5457e 100644
@@ -4,3 +4,4 @@
 /statx04
 /statx05
 /statx06
+/statx07
testcases/kernel/syscalls/statx/statx07.c new file +170 -0

170 lines not on this page · raw message

Signature · 1 line
2.1.4
#1 lore raw

PATCH v4 1/1 syscalls/statx: Add test for sync flags

Hi Vaishnavi,

Quoted 10 lines

Testcase 1: AT_STATX_DONT_SYNC
With AT_STATX_DONT_SYNC server changes should not get sync with client.

Testcase 2: AT_STATX_FORCE_SYNC
With AT_STATX_FORCE_SYNC server changes should get sync with client.

Signed-off-by Kewal Ukunde <[email protected]>
Signed-off-by Vaishnavi.d <[email protected]>
Signed-off-by Tarun.T.U <[email protected]>
Reviewed-by Petr Vorel <[email protected]>

Now reviewing it :).

...

Quoted 1 line

+ * Minimum kernel version required is 4.16.

statx() was merged into 4.11 in commit a528d35e8bfc ("statx: Add a system call to make enhanced file info available" as you had in v1. Why now requiring 4.16? It looks like second test fails now on 4.11, because calling get_mode() for it as well (see bellow).

BTW maybe .min_kver = "4.16" is self describing.

...

Quoted 1 line

+#include "old_tmpdir.h"

I think, we don't want to use old_*.h headers (although library itself lib/tst_test.c uses it). IMHO previous version with getcwd() was better.

testcases/kernel/syscalls/statx/statx07.c line 54

Quoted 6 lines
testcases/kernel/syscalls/statx/statx07.c new file +6 -0
+#define CLIENT_PATH "client"
+#define SERVER_PATH "server"
+#define CLI_FORCE_SYNC "client/force_sync_file"
+#define CLI_DONT_SYNC "client/dont_sync_file"
+#define SERV_FORCE_SYNC "server/force_sync_file"
+#define SERV_DONT_SYNC "server/dont_sync_file"

Code style: SERVER_* vs. SERV_*, CLIENT_* vs. CLI_*. Can you unify them? (pick only one variant for each?).

Quoted 1 line

+static int get_mode(char *file_name, int flag_type, char *flag_name)

I don't like passing flag and it's name, but no idea, how to avoid it.

Quoted 25 lines
testcases/kernel/syscalls/statx/statx07.c new file +24 -0
+{
+	struct statx buff;
+
+	TEST(statx(AT_FDCWD, file_name, flag_type, STATX_ALL, &buff));
+
+	if (TST_RET == -1)
+		tst_brk(TFAIL | TST_ERR,
+			"statx(AT_FDCWD, %s, %s, STATX_ALL, &buff)",
+			file_name, flag_name);
+	else
+		tst_res(TINFO, "statx(AT_FDCWD, %s, %s, STATX_ALL, &buff)",
+			file_name, flag_name);
+
+	return buff.stx_mode;
+}
 
+const struct test_cases {
+	char *server_file;
+	char *client_file;
+	char *flag_name;
+	int flag;
+	unsigned int mode;
+
+} tcases[] = {
+	{SERV_DONT_SYNC,

testcases/kernel/syscalls/statx/statx07.c line 95

Quoted 10 lines
testcases/kernel/syscalls/statx/statx07.c new file +10 -0
+	 CLI_DONT_SYNC,
+	 "AT_STATX_DONT_SYNC",
+	 AT_STATX_DONT_SYNC,
+	 DEFAULT_MODE},
+	{SERV_FORCE_SYNC,
+	 CLI_FORCE_SYNC,
+	 "AT_STATX_FORCE_SYNC",
+	 AT_STATX_FORCE_SYNC,
+	 CURRENT_MODE}
+};

flag_name and flag is duplicity.

We often use macro for this purpose:

testcases/kernel/syscalls/clock_nanosleep/clock_nanosleep01.c #define TYPE_NAME(x) .ttype = x, .desc = #x
...

static struct test_case tcase[] = {
	{
		TYPE_NAME(NORMAL),

Note about code style:
tcases[] formatting: { } should be on single line. If you can, use shorter names (it's C).

Quoted 1 line

+static void test_statx(unsigned int nr)

Maybe i?
static void test_statx(unsigned int nr)

testcases/kernel/syscalls/statx/statx07.c line 105

Quoted 8 lines
testcases/kernel/syscalls/statx/statx07.c new file +8 -0
+{
+	const struct test_cases *tc = &tcases[nr];
+	unsigned int cur_mode;
+
+	get_mode(tc->client_file, AT_STATX_FORCE_SYNC, "AT_STATX_FORCE_SYNC");
+
+	SAFE_CHMOD(tc->server_file, CURRENT_MODE);
+	cur_mode = get_mode(tc->client_file, tc->flag, tc->flag_name);

This helps to be 4.11 compatible:

	if (tc->flag == AT_STATX_DONT_SYNC)
		get_mode(tc->client_file, AT_STATX_FORCE_SYNC, "AT_STATX_FORCE_SYNC");
Quoted 9 lines
testcases/kernel/syscalls/statx/statx07.c new file +9 -0
+	if (MODE(cur_mode) == tc->mode)
+		tst_res(TPASS,
+			"statx() with %s for mode %o %o",
+			tc->flag_name, tc->mode, MODE(cur_mode));
+	else
+		tst_res(TFAIL,
+			"statx() with %s for mode %o %o",
+			tc->flag_name, tc->mode, MODE(cur_mode));
+}

...

Quoted 2 lines
(quoted diff) +2 -0
+static void setup(void)
+{

...

testcases/kernel/syscalls/statx/statx07.c line 133

Quoted 3 lines
testcases/kernel/syscalls/statx/statx07.c new file +3 -0
+	snprintf(cmd, sizeof(cmd),
+		 "exportfs -i -o no_root_squash,rw,sync,no_subtree_check *%.1024s",
+		 server_path);

Maybe whole cleanup should be skipped if exportfs fails?

	exported = 1;
Quoted 7 lines
testcases/kernel/syscalls/statx/statx07.c new file +7 -0
+
+	ret = tst_system(cmd);
+	if (WEXITSTATUS(ret) == 127)
+		tst_brk(TCONF | TST_ERR, "%s not found", cmd);
+	if (ret)
+		tst_brk(TBROK | TST_ERR, "failed to exportfs");
+

testcases/kernel/syscalls/statx/statx07.c line 150

Quoted 10 lines
testcases/kernel/syscalls/statx/statx07.c new file +9 -0
+	if (mount(server_path, CLIENT_PATH, "nfs", 0, "addr=127.0.0.1")) {
+		if (errno == EOPNOTSUPP)
+			tst_brk(TCONF | TERRNO, "nfs server not set up?");
+		tst_brk(TBROK | TERRNO, "mount() nfs failed");
+	}
+	mounted = 1;
+}
 
+static void cleanup(void)
+{

	Here skip cleanup if exportfs during setup failed:
	if (!exported)
		return;

testcases/kernel/syscalls/statx/statx07.c line 159

Quoted 9 lines
testcases/kernel/syscalls/statx/statx07.c new file +9 -0
+	snprintf(cmd, sizeof(cmd),
+		 "exportfs -u *:%s/%s", cwd, SERVER_PATH);
+
+	if (tst_system(cmd) == -1)
+		tst_res(TWARN | TST_ERR, "failed to clear exportfs");
+
+	if (mounted)
+		SAFE_UMOUNT(CLIENT_PATH);
+}

...

Kind regards,
Petr

#2 lore raw

PATCH v4 1/1 syscalls/statx: Add test for sync flags

Hi!

Quoted 3 lines
Quoted 1 line

+#include "old_tmpdir.h"

I think, we don't want to use old_*.h headers (although library itself lib/tst_test.c uses it). IMHO previous version with getcwd() was better.

My bad it does not seem to be exported to the new library, I thought it was. I will fix the headers so that it's exported and remove this include before applying this patch.

Signature · 2 lines
Cyril Hrubis
[email protected]
author
#3 lore raw

PATCH v4 1/1 syscalls/statx: Add test for sync flags

In reply to Petr Vorel

Hi,

Quoted 8 lines
Quoted 1 line

+ * Minimum kernel version required is 4.16.

statx() was merged into 4.11 in commit a528d35e8bfc ("statx: Add a system call
to make enhanced file info available" as you had in v1. Why now requiring 4.16?
It looks like second test fails now on 4.11, because calling get_mode() for it
as well (see bellow).

Actually, support for sync flags was added only from kernel version 4.16. Since, I was using 4.16, I didn't face that issue. Thats why minimum kernel version has been updated to 4.16 in the patch v5.
(https://github.com/torvalds/linux/commit/9ccee940bd5b766b6dab6c1a80908b9490a4850d)

Quoted 4 lines
Quoted 1 line

+#include "old_tmpdir.h"

I think, we don't want to use old_*.h headers (although library itself lib/tst_test.c uses it). IMHO previous version with getcwd() was better.

The function get tempdir will be ported to the new library soon. So, I have not modified that part. Other changes have been made in patch v5.

Regards,
Vaishnavi D

#4 lore raw

PATCH v4 1/1 syscalls/statx: Add test for sync flags

Hi Vaishnavi,

Quoted 4 lines

Actually, support for sync flags was added only from kernel version 4.16. Since, I was using 4.16, I didn't face that issue. Thats why minimum kernel version has been updated to 4.16 in the patch v5.
(https://github.com/torvalds/linux/commit/9ccee940bd5b766b6dab6c1a80908b9490a4850d)

Thanks for update.

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

+#include "old_tmpdir.h"

I think, we don't want to use old_*.h headers (although library itself lib/tst_test.c uses it). IMHO previous version with getcwd() was better.

The function get tempdir will be ported to the new library soon. So, I have not modified that part. Other changes have been made in patch v5.

Good, I noticed Cyril's intention to fix the headers. We'll just wait for it and remove import old_tmpdir.h before merging v5.

Quoted 2 lines

Regards,
Vaishnavi D

Kind regards,
Petr