From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1WAN1I-0004qo-OY for ltp-list@lists.sourceforge.net; Mon, 03 Feb 2014 17:09:16 +0000 Date: Mon, 3 Feb 2014 18:08:43 +0100 From: chrubis@suse.cz Message-ID: <20140203170843.GA4298@rei> References: <1390298574-2966-1-git-send-email-wangxg.fnst@cn.fujitsu.com> <1390298574-2966-3-git-send-email-wangxg.fnst@cn.fujitsu.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1390298574-2966-3-git-send-email-wangxg.fnst@cn.fujitsu.com> Subject: Re: [LTP] [PATCH v2 3/3] fcntl/fcntl31.c: add I/O availability signals test for fcntl(2) List-Id: Linux Test Project General Discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ltp-list-bounces@lists.sourceforge.net To: Xiaoguang Wang Cc: ltp-list@lists.sourceforge.net Hi! > create a new case to test F_GETOWN, F_SETOWN, F_GETOWN_EX, > F_SETOWN_EX, F_GETSIG, F_SETSIG for fcntl(2) > > Signed-off-by: Xiaoguang Wang > --- > m4/ltp-fcntl.m4 | 46 ++++ > runtest/ltplite | 1 + > runtest/stress.part3 | 1 + > runtest/syscalls | 2 + > testcases/kernel/syscalls/.gitignore | 2 + > testcases/kernel/syscalls/fcntl/fcntl31.c | 426 ++++++++++++++++++++++++++++++ > 6 files changed, 478 insertions(+) > create mode 100644 m4/ltp-fcntl.m4 > create mode 100644 testcases/kernel/syscalls/fcntl/fcntl31.c > > diff --git a/m4/ltp-fcntl.m4 b/m4/ltp-fcntl.m4 > new file mode 100644 > index 0000000..2ec284b > --- /dev/null > +++ b/m4/ltp-fcntl.m4 > @@ -0,0 +1,46 @@ > +dnl > +dnl Copyright (c) 2014 Fujitsu Ltd. > +dnl Author: Xiaoguang Wang > +dnl > +dnl This program is free software; you can redistribute it and/or modify > +dnl it under the terms of the GNU General Public License as published by > +dnl the Free Software Foundation; either version 2 of the License, or > +dnl (at your option) any later version. > +dnl > +dnl This program is distributed in the hope that it will be useful, > +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of > +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See > +dnl the GNU General Public License for more details. > +dnl > +dnl You should have received a copy of the GNU General Public License > +dnl along with this program; if not, write to the Free Software > +dnl Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > +dnl > + > +dnl > +dnl LTP_CHECK_SYSCALL_FCNTL > +dnl ---------------------------- > +dnl > +AC_DEFUN([LTP_CHECK_SYSCALL_FCNTL],[dnl > + AC_MSG_CHECKING([for fcntl f_owner_ex]) > + AC_LINK_IFELSE([AC_LANG_SOURCE([ > +#define _GNU_SOURCE > +#include > +#include > +#include > +int main(void) { > + struct f_owner_ex tst_own_ex; > + FILE *fp; > + > + fp = tmpfile(); > + fcntl(fileno(fp), F_GETOWN_EX, &tst_own_ex); > + return 0; I guess that simpler: AC_COMPILE_IFELSE([AC_LANG_SOURCE([ #define _GNU_SOURCE #include int main(void) { struct f_owner_ex tst_own_ex; return 0; } ])], ... Should work as well. > +}])],[has_f_owner_ex="yes"]) > + > +if test "x$has_f_owner_ex" = xyes; then > + AC_DEFINE(HAVE_STRUCT_F_OWNER_EX,1,[Define to 1 if you have struct f_owner_ex]) > + AC_MSG_RESULT(yes) > +else > + AC_MSG_RESULT(no) > +fi > +]) Also shouldn't be this check added to configure.ac (similary to the rest of them)? > diff --git a/runtest/ltplite b/runtest/ltplite > index 80e9527..dff3542 100644 > --- a/runtest/ltplite > +++ b/runtest/ltplite > @@ -225,6 +225,7 @@ fcntl26 fcntl26 > # fcntl28 fcntl28 > fcntl29 fcntl29 > fcntl30 fcntl30 > +fcntl31 fcntl31 > > fdatasync01 fdatasync01 > fdatasync02 fdatasync02 > diff --git a/runtest/stress.part3 b/runtest/stress.part3 > index d593f92..906cbdd 100644 > --- a/runtest/stress.part3 > +++ b/runtest/stress.part3 > @@ -164,6 +164,7 @@ fcntl26 fcntl26 > # fcntl28 fcntl28 > fcntl29 fcntl29 > fcntl30 fcntl30 > +fcntl31 fcntl31 > > fdatasync01 fdatasync01 > fdatasync02 fdatasync02 > diff --git a/runtest/syscalls b/runtest/syscalls > index eec05a9..f775a36 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -256,6 +256,8 @@ fcntl29 fcntl29 > fcntl29_64 fcntl29_64 > fcntl30 fcntl30 > fcntl30_64 fcntl30_64 > +fcntl31 fcntl31 > +fcntl31_64 fcntl31_64 > > fdatasync01 fdatasync01 > fdatasync02 fdatasync02 > diff --git a/testcases/kernel/syscalls/.gitignore b/testcases/kernel/syscalls/.gitignore > index a5a999d..e616bdc 100644 > --- a/testcases/kernel/syscalls/.gitignore > +++ b/testcases/kernel/syscalls/.gitignore > @@ -218,6 +218,8 @@ > /fcntl/fcntl29_64 > /fcntl/fcntl30 > /fcntl/fcntl30_64 > +/fcntl/fcntl31 > +/fcntl/fcntl31_64 > /fdatasync/fdatasync01 > /fdatasync/fdatasync02 > /flock/flock01 > diff --git a/testcases/kernel/syscalls/fcntl/fcntl31.c b/testcases/kernel/syscalls/fcntl/fcntl31.c > new file mode 100644 > index 0000000..5943385 > --- /dev/null > +++ b/testcases/kernel/syscalls/fcntl/fcntl31.c > @@ -0,0 +1,426 @@ > +/* > + * Copyright (c) 2014 Fujitsu Ltd. > + * Author: Xiaoguang Wang > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of version 2 of the GNU General Public License as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it would be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > + > +/* > + * Description: > + * Verify that: > + * Basic test for fcntl(2) using F_GETOWN, F_SETOWN, F_GETOWN_EX, > + * F_SETOWN_EX, F_GETSIG, F_SETSIG argument. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "test.h" > +#include "config.h" > +#include "usctest.h" > +#include "linux_syscall_numbers.h" > +#include "safe_macros.h" > + > + > +static void setup(void); > +static void cleanup(void); > + > +static void setown_pid_test(void); > +static void setown_pgrp_test(void); > + > +static void set_get_own_cleanup(void); > + > +#if defined(HAVE_STRUCT_F_OWNER_EX) > +static int setownex_pid_setup(void); > +static int setownex_pgrp_setup(void); > +static int setownex_tid_setup(void); > + > +static void setownex_test(void); > +static void setownex_cleanup(void); > + > +static struct f_owner_ex tst_own_ex; > +static struct f_owner_ex orig_own_ex; > +#endif > + > +static void sig_func(int signo); > + > +static void signal_parent(void); > +static void wait_io_ready(void); > +static void test_set_and_get_sig(int sig); > + > +static pid_t pid; > +static pid_t orig_pid; > +static pid_t pgrp_pid; > + > +static int test_fd; > +static int pipe_fds[2]; > +static int ind; > +static int ownex_enabled; > + > +/* record the io events signal */ > +static int io_signal_received; This should be volatile sig_atomic_t > + > + > +static struct test_case_t { > + int cmd; > + int (*setup)(void); > + void (*testfunc)(void); > + void (*cleanup)(void); > + char *des; > +} test_cases[] = { > + {F_SETOWN, NULL, setown_pid_test, set_get_own_cleanup, > + "F_SETOWN for process ID"}, > + {F_SETOWN, NULL, setown_pgrp_test, set_get_own_cleanup, > + "F_SETOWN for process group ID"}, > +#if defined(HAVE_STRUCT_F_OWNER_EX) > + {F_SETOWN_EX, setownex_tid_setup, setownex_test, > + setownex_cleanup, "F_SETOWN_EX for thread ID"}, > + {F_SETOWN_EX, setownex_pid_setup, setownex_test, > + setownex_cleanup, "F_SETOWN_EX for process ID"}, > + {F_SETOWN_EX, setownex_pgrp_setup, setownex_test, > + setownex_cleanup, "F_SETOWN_EX for process group ID"}, > +#endif > +}; > + > +char *TCID = "fcntl31"; > +int TST_TOTAL = ARRAY_SIZE(test_cases); > + > + > +int main(int ac, char **av) > +{ > + int lc; > + char *msg; > + int i; > + > + msg = parse_opts(ac, av, NULL, NULL); > + if (msg != NULL) > + tst_brkm(TBROK, NULL, "OPTION PARSING ERROR - %s", msg); > + > + setup(); > + > + for (lc = 0; TEST_LOOPING(lc); lc++) { > + tst_count = 0; > + > + ind = -1; > + for (i = 0; i < TST_TOTAL; i++) { > + ind++; This is ugly, if the testcase needs to get something from it's test structure make the testfunc prototype to: void (*testfunc)(struct test_case_t *self) And pass the pointer to the structure to it. > + if (test_cases[i].setup && test_cases[i].setup() != 0) > + continue; > + > + if (test_cases[i].testfunc) > + test_cases[i].testfunc(); I would drop this if, a test without test function is nonsense. > + if (test_cases[i].cleanup) > + test_cases[i].cleanup(); Also I wonder if it makes sense to create a test_case_t structure and fill it with information when the main loop just calls a function. You can do the same just with an array of functions... > + } > + } > + > + cleanup(); > + tst_exit(); > +} > + > +static void setup(void) > +{ > + int ret; > + > + tst_sig(FORK, DEF_HANDLER, cleanup); > + > + tst_tmpdir(); > + > + TEST_PAUSE; > + > + /* we have the tests on pipe */ > + SAFE_PIPE(cleanup, pipe_fds); > + test_fd = pipe_fds[0]; > + if (fcntl(test_fd, F_SETFL, O_ASYNC) < 0) > + tst_brkm(TBROK | TERRNO, cleanup, "fcntl set O_ASYNC failed"); > + > + pid = getpid(); > + > + ret = setpgrp(); > + if (ret < 0) > + tst_brkm(TBROK | TERRNO, cleanup, "setpgrp() failed"); > + pgrp_pid = getpgid(0); > + if (pgrp_pid < 0) > + tst_brkm(TBROK | TERRNO, cleanup, "getpgid() failed"); > + > +#if defined(HAVE_STRUCT_F_OWNER_EX) > + if ((tst_kvercmp(2, 6, 32)) < 0) { > + tst_resm(TCONF, "F_GETOWN_EX and F_GETOWN_EX only run on " > + "kernels that are 2.6.32 and higher"); > + } else { > + ownex_enabled = 1; > + > + /* get original f_owner_ex info */ > + TEST(fcntl(test_fd, F_GETOWN_EX, &orig_own_ex)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl get original f_owner_ex info failed"); > + } > + } > +#endif > + > + /* get original pid info */ > + TEST(fcntl(test_fd, F_GETOWN)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl get original pid info failed"); > + } > + orig_pid = TEST_RETURN; > + > + if (signal(SIGIO, sig_func) == SIG_ERR) { > + tst_brkm(TBROK | TERRNO, cleanup, > + "set signal handler for SIGIO failed"); > + } > + if (signal(SIGUSR1, sig_func) == SIG_ERR) { > + tst_brkm(TBROK | TERRNO, cleanup, > + "set signal handler for SIGUSR1 failed"); > + } > +} > + > +static void sig_func(int signo) > +{ > + int ret; > + pid_t signaled_pid; > + char c; > + > + signaled_pid = getpid(); > + > + /* > + * just check the signal received in parent process, in case > + * signal is sent to a process group. > + */ > + if (signaled_pid != pid) > + return; > + > + switch (signo) { > + case SIGUSR1: > + io_signal_received = SIGUSR1; > + break; > + case SIGIO: > + io_signal_received = SIGIO; > + } > + > + ret = read(pipe_fds[0], &c, 1); > + if (ret < 0) > + tst_resm(TINFO, "no data is read"); You should not call anything that is not signal-async-safe from signal handler. The tst_resm() is not signal-async-safe. > +} > + > +static void setown_pid_test(void) > +{ > + TEST(fcntl(test_fd, F_SETOWN, pid)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETOWN set process id failed"); > + } > + test_set_and_get_sig(SIGUSR1); > +} > + > +static void setown_pgrp_test(void) > +{ > + TEST(fcntl(test_fd, F_SETOWN, -pgrp_pid)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETOWN set process group id failed"); > + } > + test_set_and_get_sig(SIGUSR1); > +} > + > +static void set_get_own_cleanup(void) > +{ > + TEST(fcntl(test_fd, F_SETOWN, orig_pid)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETOWN restore orig_pid failed"); > + } > +} > + > +#if defined(HAVE_STRUCT_F_OWNER_EX) > +static int setownex_pid_setup(void) > +{ > + if (ownex_enabled == 0) > + return 1; > + > + tst_own_ex.type = F_OWNER_PID; > + tst_own_ex.pid = pid; > + return 0; > +} > + > +static void setownex_test(void) > +{ > + TEST(fcntl(test_fd, F_SETOWN_EX, &tst_own_ex)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETOWN_EX failed"); > + } > + test_set_and_get_sig(SIGUSR1); > +} > + > +static void setownex_cleanup(void) > +{ > + TEST(fcntl(test_fd, F_SETOWN_EX, &orig_own_ex)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETOWN_EX restore orig_own_ex failed"); > + } > +} > + > +static int setownex_pgrp_setup(void) > +{ > + if (ownex_enabled == 0) > + return 1; > + > + tst_own_ex.type = F_OWNER_PGRP; > + tst_own_ex.pid = pgrp_pid; > + return 0; > +} > + > +static int setownex_tid_setup(void) > +{ > + pid_t tid; > + > + if (ownex_enabled == 0) Shouldn't we rather print the TCONF here than in the global setup? something this in global setup: if (kernel_too_old) { ownex_enabled = 0; ownex_tconf_msg = "F_GETOWN_EX and F_GETOWN_EX needs kernel 2.6.32+" } and something as this here: tst_resm(TCONF, ownex_tconf_msg); because otherwise the number of tests for the testcase and number of passes/fails/tconfs does not add up. > + return 1; > + > + tid = syscall(__NR_gettid); > + tst_own_ex.type = F_OWNER_TID; > + tst_own_ex.pid = tid; > + return 0; > +} > +#endif > + > +static void test_set_and_get_sig(int sig) > +{ > + int orig_sig; > + > + TEST(fcntl(test_fd, F_GETSIG)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl(fd, F_GETSIG) get orig_sig failed"); > + } > + orig_sig = TEST_RETURN; > + > + if (orig_sig == 0 || orig_sig == SIGIO) > + tst_resm(TINFO, "default io events signal is SIGIO"); > + > + TEST(fcntl(test_fd, F_SETSIG, sig)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl(fd, F_SETSIG, SIG: %d) failed", sig); > + } > + > + TEST(fcntl(test_fd, F_GETSIG)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl(fd, F_GETSIG) get the set signal failed"); > + } > + if (TEST_RETURN != sig) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl F_SETSIG set SIG: %d failed", sig); > + } > + > + wait_io_ready(); > + > + /* restore the default signal*/ > + TEST(fcntl(test_fd, F_SETSIG, orig_sig)); > + if (TEST_RETURN < 0) { > + tst_brkm(TFAIL | TTERRNO, cleanup, > + "fcntl restore default signal failed"); > + } > +} > + > +static void signal_parent(void) > +{ > + int ret, fd; > + > + fd = pipe_fds[1]; > + close(pipe_fds[0]); > + > + ret = write(fd, "c", 1); > + > + switch (ret) { > + case 0: > + fprintf(stderr, "No data written, something is wrong"); Missing newline at the end of string > + break; > + case -1: > + fprintf(stderr, "Failed to write to pipe: %s\n", > + strerror(errno)); > + } > + > + close(fd); > + return; > +} > + > +static void wait_io_ready(void) > +{ > + pid_t child; > + int i; > + > + fflush(stdout); You should use tst_flush() or tst_fork() instead. Note: I've just added the tst_fork() and updated the test writing guidelines. > + child = FORK_OR_VFORK(); The rest of the testcase would not run on uClinux so there is no reason to use FORK_OR_VFORK() instead of fork(). > + if (child < 0) > + tst_brkm(TBROK | TERRNO, cleanup, "fork failed"); > + > + if (child == 0) { > + signal_parent(); > + exit(0); > + } else { > + /* > + * if io_signal_received is not changed by signal handler > + * in 5 seconds, consider the test failed. > + */ > + for (i = 0; i < 1000; i++) { > + /* SIGUSR1 or SIGIO is received*/ > + if (io_signal_received > 0) > + break; > + sched_yield(); > + usleep(5000); > + } You can use sigtimedwait() instead :) > + switch (io_signal_received) { > + case SIGUSR1: > + tst_resm(TPASS, "fcntl test %s success", > + test_cases[ind].des); > + break; > + case SIGIO: > + tst_resm(TFAIL, "received default SIGIO, fcntl test " > + "%s failed", test_cases[ind].des); > + break; > + default: > + tst_brkm(TBROK, cleanup, "fcntl io events " > + "signal mechanism work abnormally"); > + } > + io_signal_received = 0; > + wait(NULL); > + } > +} > + > +static void cleanup(void) > +{ > + TEST_CLEANUP; > + > + if (pipe_fds[0] > 0) > + SAFE_CLOSE(NULL, pipe_fds[0]); > + if (pipe_fds[1] > 0) > + SAFE_CLOSE(NULL, pipe_fds[1]); > + > + tst_rmdir(); > +} -- Cyril Hrubis chrubis@suse.cz ------------------------------------------------------------------------------ Managing the Performance of Cloud-Based Applications Take advantage of what the Cloud has to offer - Avoid Common Pitfalls. Read the Whitepaper. http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk _______________________________________________ Ltp-list mailing list Ltp-list@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ltp-list