| Message ID | 20180917075248.28004-1-kewal@zilogic.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v2] Test statx syscall for SYNC_FLAGS | expand |
Hi Kewal, LGTM, bellow are some minor enhancement tips. > * statx06.c :- This could be deleted. Maybe first line of commit message "syscalls/statx: Add test for sync flags" (SYNC_FLAGS looks like a constant, which is misleading). > 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 <kewal@zilogic.com> > Signed-off-by: Vaishnavi.d <vaishnavi.d@zilogic.com> > Signed-off-by: Tarun.T.U <tarun@zilogic.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> > --- > runtest/syscalls | 1 + > testcases/kernel/syscalls/statx/.gitignore | 1 + > testcases/kernel/syscalls/statx/statx06.c | 195 +++++++++++++++++++++++++++++ > 3 files changed, 197 insertions(+) > create mode 100644 testcases/kernel/syscalls/statx/statx06.c It's needed to rename statx06.c to statx07.c (as previous was taken). ... > diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c ... > +static void test_for_dont_sync(void) > +{ > + unsigned int cur_mode; > + char server_path[F_PATH_SIZE]; > + > + snprintf(server_path, sizeof(server_path), "%s/%s", SERVER_PATH, > + DONT_SYNC_FILE); > + > + get_mode(DONT_SYNC_FILE, AT_STATX_FORCE_SYNC, > + "AT_STATX_FORCE_SYNC"); > + > + SAFE_CHMOD(server_path, CURRENT_MODE); > + cur_mode = get_mode(DONT_SYNC_FILE, AT_STATX_DONT_SYNC, > + "AT_STATX_DONT_SYNC"); > + > + if (MODE(cur_mode) == DEFAULT_MODE) > + tst_res(TPASS, > + "statx() with AT_STATX_DONT_SYNC for mode %o %o", > + DEFAULT_MODE, MODE(cur_mode)); > + else > + tst_res(TFAIL, > + "statx() with AT_STATX_DONT_SYNC for mode %o %o", > + DEFAULT_MODE, MODE(cur_mode)); > +} > + > +static void test_for_force_sync(void) > +{ > + unsigned int cur_mode; > + char server_path[F_PATH_SIZE]; > + > + snprintf(server_path, sizeof(server_path), "%s/%s", SERVER_PATH, > + FORCE_SYNC_FILE); > + > + SAFE_CHMOD(server_path, CURRENT_MODE); > + cur_mode = get_mode(FORCE_SYNC_FILE, AT_STATX_FORCE_SYNC, > + "AT_STATX_FORCE_SYNC"); > + > + if (MODE(cur_mode) == CURRENT_MODE) > + tst_res(TPASS, > + "statx() with AT_STATX_FORCE_SYNC for mode %o %o", > + CURRENT_MODE, MODE(cur_mode)); > + else > + tst_res(TFAIL, > + "statx() with AT_STATX_FORCE_SYNC for mode %o %o", > + CURRENT_MODE, MODE(cur_mode)); > +} > + > +const struct test_cases { > + void (*func)(void); > +} tcases[] = { > + {test_for_dont_sync}, > + {test_for_force_sync} > +}; Both testing functions are nearly the same, why to repeat yourself? How about, instead of passing test function pointer, pass mode and other needed parameters to single test function? > + > +static void test_statx(unsigned int nr) > +{ > + const struct test_cases *tc = &tcases[nr]; > + > + tc->func(); > +} I suggest other changes (see following diff): * Add mount flag to avoid false positive warning: safe_macros.c:773: WARN: statx07.c:181: umount(client) failed: EINVAL * Fix gcc-8 format-truncation error (1024 is far too much, it could be less). * Use tst_res(TWARN, ...) for exportfs. Kind regards, Petr diff --git testcases/kernel/syscalls/statx/statx07.c testcases/kernel/syscalls/statx/statx07.c index f64322d7d..ff988fd00 100644 --- testcases/kernel/syscalls/statx/statx07.c +++ testcases/kernel/syscalls/statx/statx07.c @@ -51,6 +51,7 @@ static char cwd[PATH_MAX]; static char cmd[BUFF_SIZE]; +static int mounted; static int get_mode(char *file_name, int flag_type, char *flag_name) { @@ -163,7 +162,7 @@ static void setup(void) snprintf(mount_data, sizeof(mount_data), "addr=%s", ip); snprintf(cmd, sizeof(cmd), - "exportfs -i -o no_root_squash,rw,sync,no_subtree_check *%s", + "exportfs -i -o no_root_squash,rw,sync,no_subtree_check *%.1024s", server_path); ret = tst_system(cmd); @@ -173,14 +172,16 @@ static void setup(void) tst_brk(TBROK | TST_ERR, "failed to exportfs"); SAFE_MOUNT(server_path, client_path, "nfs", 0, mount_data); + mounted = 1; } static void cleanup(void) { if (tst_system("exportfs -ua") == -1) - tst_brk(TBROK | TST_ERR, "failed to clear exportfs"); + tst_res(TWARN | TST_ERR, "failed to clear exportfs"); - SAFE_UMOUNT(CLIENT_PATH); + if (mounted) + SAFE_UMOUNT(CLIENT_PATH); } static struct tst_test test = {
Hi Kewal, > +++ b/testcases/kernel/syscalls/statx/statx07.c ... > + * Minimum kernel version required is 4.11. > + */ One more thing, you need to define _GNU_SOURCE here as it's needed for glibc and thus used for detection in m4/ltp-statx.m4. #define _GNU_SOURCE > +#include <netdb.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <linux/limits.h> > +#include <sys/mount.h> > +#include "tst_test.h" > +#include "lapi/stat.h" Without it it fails on glibc >= 2.28 https://travis-ci.org/pevik/ltp/builds/470077800 I thing it'd be better to remove _GNU_SOURCE from statx0*.c and put it into lapi/stat.h. Kind regards, Petr
Hi, > Hi Kewal, > >> +++ b/testcases/kernel/syscalls/statx/statx07.c > ... >> + * Minimum kernel version required is 4.11. >> + */ > > One more thing, you need to define _GNU_SOURCE here as it's needed for > glibc and > thus used for detection in m4/ltp-statx.m4. > > > #define _GNU_SOURCE > >> +#include <netdb.h> >> +#include <stdio.h> >> +#include <stdlib.h> >> +#include <linux/limits.h> >> +#include <sys/mount.h> >> +#include "tst_test.h" >> +#include "lapi/stat.h" > > Without it it fails on glibc >= 2.28 > https://travis-ci.org/pevik/ltp/builds/470077800 > > I thing it'd be better to remove _GNU_SOURCE from statx0*.c and put it > into > lapi/stat.h. These changes have already been made in v5 of this test case. This patch has been renamed to syscalls/statx: Add test for sync flags. Waiting for the tst_get_tmpdir() function to be ported to new library for the patch to be merged. Regards, Vaishnavi D
Hi Kewal, > > One more thing, you need to define _GNU_SOURCE here as it's needed for > > glibc and > > thus used for detection in m4/ltp-statx.m4. > > #define _GNU_SOURCE > >> +#include <netdb.h> > >> +#include <stdio.h> > >> +#include <stdlib.h> > >> +#include <linux/limits.h> > >> +#include <sys/mount.h> > >> +#include "tst_test.h" > >> +#include "lapi/stat.h" > > Without it it fails on glibc >= 2.28 > > https://travis-ci.org/pevik/ltp/builds/470077800 > > I thing it'd be better to remove _GNU_SOURCE from statx0*.c and put it > > into > > lapi/stat.h. > These changes have already been made in v5 of this test case. This patch > has been renamed to syscalls/statx: Add test for sync flags. Waiting for > the tst_get_tmpdir() function to be ported to new library for the patch to > be merged. ok, I'm sorry, I got confused as you're both working on the same test. But the problem remain even on v5 [1]: missing _GNU_SOURCE definition while using lapi/stat.h. > Regards, > Vaishnavi D Kind regards, Petr [1] https://patchwork.ozlabs.org/patch/1011904/
Hi, > ok, I'm sorry, I got confused as you're both working on the same test. > But the problem remain even on v5 [1]: missing _GNU_SOURCE definition > while > using lapi/stat.h. Sorry, I failed to check that. Have added _GNU_SOURCE definition in lapi/stat.h in patch v6. Regards, Vaishnavi D
diff --git a/runtest/syscalls b/runtest/syscalls index 0d0be7713..71b83b25c 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1504,3 +1504,4 @@ statx02 statx02 statx03 statx03 statx04 statx04 statx05 statx05 +statx06 statx06 diff --git a/testcases/kernel/syscalls/statx/.gitignore b/testcases/kernel/syscalls/statx/.gitignore index 209fc3a33..c1ceddabf 100644 --- a/testcases/kernel/syscalls/statx/.gitignore +++ b/testcases/kernel/syscalls/statx/.gitignore @@ -3,3 +3,4 @@ /statx03 /statx04 /statx05 +/statx06 diff --git a/testcases/kernel/syscalls/statx/statx06.c b/testcases/kernel/syscalls/statx/statx06.c new file mode 100644 index 000000000..f64322d7d --- /dev/null +++ b/testcases/kernel/syscalls/statx/statx06.c @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) Zilogic Systems Pvt. Ltd., 2018 + * Email : code@zilogic.com + */ + +/* + * Test statx + * + * This code tests the following flags: + * 1) AT_STATX_FORCE_SYNC + * 2) AT_STATX_DONT_SYNC + * + * By exportfs cmd creating NFS setup. + * + * A test file is created in server folder and statx is being + * done in client folder. + * + * TESTCASE 1: + * BY AT_STATX_SYNC_AS_STAT getting predefined mode value. + * Then, by using AT_STATX_FORCE_SYNC getting new updated vaue + * from server file changes. + * + * TESTCASE 2: + * BY AT_STATX_SYNC_AS_STAT getting predefined mode value. + * AT_STATX_FORCE_SYNC is called to create cache data of the file. + * Then, by using DONT_SYNC_FILE getting old cached data in client folder, + * but mode has been chaged in server file. + * + * Minimum kernel version required is 4.11. + */ + +#include <netdb.h> +#include <stdio.h> +#include <stdlib.h> +#include <linux/limits.h> +#include <sys/mount.h> +#include "tst_test.h" +#include "lapi/stat.h" + +#define MODE(X) (X & (~S_IFMT)) +#define F_PATH_SIZE 50 +#define BUFF_SIZE (PATH_MAX + 50) +#define DEFAULT_MODE 0644 +#define CURRENT_MODE 0777 + +#define CLIENT_PATH "client" +#define SERVER_PATH "server" +#define FORCE_SYNC_FILE "force_sync_file" +#define DONT_SYNC_FILE "dont_sync_file" + +static char cwd[PATH_MAX]; +static char cmd[BUFF_SIZE]; + +static int get_mode(char *file_name, int flag_type, char *flag_name) +{ + char client_path[F_PATH_SIZE]; + struct statx buff; + + snprintf(client_path, sizeof(client_path), "%s/%s", + CLIENT_PATH, file_name); + + TEST(statx(AT_FDCWD, client_path, 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; +} + +static void test_for_dont_sync(void) +{ + unsigned int cur_mode; + char server_path[F_PATH_SIZE]; + + snprintf(server_path, sizeof(server_path), "%s/%s", SERVER_PATH, + DONT_SYNC_FILE); + + get_mode(DONT_SYNC_FILE, AT_STATX_FORCE_SYNC, + "AT_STATX_FORCE_SYNC"); + + SAFE_CHMOD(server_path, CURRENT_MODE); + cur_mode = get_mode(DONT_SYNC_FILE, AT_STATX_DONT_SYNC, + "AT_STATX_DONT_SYNC"); + + if (MODE(cur_mode) == DEFAULT_MODE) + tst_res(TPASS, + "statx() with AT_STATX_DONT_SYNC for mode %o %o", + DEFAULT_MODE, MODE(cur_mode)); + else + tst_res(TFAIL, + "statx() with AT_STATX_DONT_SYNC for mode %o %o", + DEFAULT_MODE, MODE(cur_mode)); +} + +static void test_for_force_sync(void) +{ + unsigned int cur_mode; + char server_path[F_PATH_SIZE]; + + snprintf(server_path, sizeof(server_path), "%s/%s", SERVER_PATH, + FORCE_SYNC_FILE); + + SAFE_CHMOD(server_path, CURRENT_MODE); + cur_mode = get_mode(FORCE_SYNC_FILE, AT_STATX_FORCE_SYNC, + "AT_STATX_FORCE_SYNC"); + + if (MODE(cur_mode) == CURRENT_MODE) + tst_res(TPASS, + "statx() with AT_STATX_FORCE_SYNC for mode %o %o", + CURRENT_MODE, MODE(cur_mode)); + else + tst_res(TFAIL, + "statx() with AT_STATX_FORCE_SYNC for mode %o %o", + CURRENT_MODE, MODE(cur_mode)); +} + +const struct test_cases { + void (*func)(void); +} tcases[] = { + {test_for_dont_sync}, + {test_for_force_sync} +}; + +static void test_statx(unsigned int nr) +{ + const struct test_cases *tc = &tcases[nr]; + + tc->func(); +} + +static void setup(void) +{ + int ret; + char force_sync_file[F_PATH_SIZE]; + char dont_sync_file[F_PATH_SIZE]; + char mount_data[F_PATH_SIZE]; + char server_path[BUFF_SIZE]; + char client_path[BUFF_SIZE]; + char *ip = "127.0.0.1"; + + TESTPTR(getcwd(cwd, PATH_MAX)); + if (TST_RET_PTR == NULL) + tst_brk(TBROK | TST_ERR, "Failed to get PWD"); + + snprintf(force_sync_file, sizeof(force_sync_file), "%s/%s", + SERVER_PATH, FORCE_SYNC_FILE); + snprintf(dont_sync_file, sizeof(dont_sync_file), "%s/%s", + SERVER_PATH, DONT_SYNC_FILE); + + SAFE_MKDIR(SERVER_PATH, DEFAULT_MODE); + SAFE_MKDIR(CLIENT_PATH, DEFAULT_MODE); + SAFE_CREAT(force_sync_file, DEFAULT_MODE); + SAFE_CREAT(dont_sync_file, DEFAULT_MODE); + + snprintf(server_path, sizeof(server_path), ":%s/%s", cwd, SERVER_PATH); + snprintf(client_path, sizeof(client_path), "%s/%s", cwd, CLIENT_PATH); + snprintf(mount_data, sizeof(mount_data), "addr=%s", ip); + + snprintf(cmd, sizeof(cmd), + "exportfs -i -o no_root_squash,rw,sync,no_subtree_check *%s", + server_path); + + ret = tst_system(cmd); + if (WEXITSTATUS(ret) == 127) + tst_brk(TCONF | TST_ERR, "%s not found", cmd); + if (ret == -1) + tst_brk(TBROK | TST_ERR, "failed to exportfs"); + + SAFE_MOUNT(server_path, client_path, "nfs", 0, mount_data); +} + +static void cleanup(void) +{ + if (tst_system("exportfs -ua") == -1) + tst_brk(TBROK | TST_ERR, "failed to clear exportfs"); + + SAFE_UMOUNT(CLIENT_PATH); +} + +static struct tst_test test = { + .tcnt = ARRAY_SIZE(tcases), + .test = test_statx, + .setup = setup, + .cleanup = cleanup, + .min_kver = "4.11", + .needs_tmpdir = 1, + .dev_fs_type = "nfs", + .needs_root = 1, +};