| Message ID | 1603691317-22811-1-git-send-email-xuyang2018.jy@cn.fujitsu.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/4] syscalls/sync01: Remove it | expand |
Hi! Ping. I think this patchset is simple. Best Regards Yang Xu > This case tests whether sync() can return the correct value. But as man-page > said "sync() is always successful". So this case is meaningless > and remove it. > > Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com> > --- > runtest/syscalls | 1 - > testcases/kernel/syscalls/sync/.gitignore | 1 - > testcases/kernel/syscalls/sync/sync01.c | 182 ---------------------- > 3 files changed, 184 deletions(-) > delete mode 100644 testcases/kernel/syscalls/sync/sync01.c > > diff --git a/runtest/syscalls b/runtest/syscalls > index 0443f9f3d..2e7108655 100644 > --- a/runtest/syscalls > +++ b/runtest/syscalls > @@ -1477,7 +1477,6 @@ symlink05 symlink05 > #symlinkat test cases > symlinkat01 symlinkat01 > > -sync01 sync01 > sync02 sync02 > sync03 sync03 > > diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore > index 04f4710dd..d006746c2 100644 > --- a/testcases/kernel/syscalls/sync/.gitignore > +++ b/testcases/kernel/syscalls/sync/.gitignore > @@ -1,3 +1,2 @@ > -/sync01 > /sync02 > /sync03 > diff --git a/testcases/kernel/syscalls/sync/sync01.c b/testcases/kernel/syscalls/sync/sync01.c > deleted file mode 100644 > index dd0a336c2..000000000 > --- a/testcases/kernel/syscalls/sync/sync01.c > +++ /dev/null > @@ -1,182 +0,0 @@ > -/* > - * Copyright (c) 2000 Silicon Graphics, Inc. All Rights Reserved. > - * > - * 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. > - * > - * Further, this software is distributed without any warranty that it is > - * free of the rightful claim of any third person regarding infringement > - * or the like. Any license provided herein, whether implied or > - * otherwise, applies only to this software file. Patent licenses, if > - * any, provided herein do not apply to combinations of this program with > - * other software, or any other product whatsoever. > - * > - * 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. > - * > - * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy, > - * Mountain View, CA 94043, or: > - * > - * http://www.sgi.com > - * > - * For further information regarding this notice, see: > - * > - * http://oss.sgi.com/projects/GenInfo/NoticeExplan/ > - * > - */ > -/* $Id: sync01.c,v 1.6 2009/11/02 13:57:19 subrata_modak Exp $ */ > -/********************************************************** > - * > - * OS Test - Silicon Graphics, Inc. > - * > - * TEST IDENTIFIER : sync01 > - * > - * EXECUTED BY : anyone > - * > - * TEST TITLE : Basic test for sync(2) > - * > - * PARENT DOCUMENT : usctpl01 > - * > - * TEST CASE TOTAL : 1 > - * > - * WALL CLOCK TIME : 1 > - * > - * CPU TYPES : ALL > - * > - * AUTHOR : William Roske > - * > - * CO-PILOT : Dave Fenner > - * > - * DATE STARTED : 03/30/92 > - * > - * INITIAL RELEASE : UNICOS 7.0 > - * > - * TEST CASES > - * > - * 1.) sync(2) returns...(See Description) > - * > - * INPUT SPECIFICATIONS > - * The standard options for system call tests are accepted. > - * (See the parse_opts(3) man page). > - * > - * OUTPUT SPECIFICATIONS > - * > - * DURATION > - * Terminates - with frequency and infinite modes. > - * > - * SIGNALS > - * Uses SIGUSR1 to pause before test if option set. > - * (See the parse_opts(3) man page). > - * > - * RESOURCES > - * None > - * > - * ENVIRONMENTAL NEEDS > - * No run-time environmental needs. > - * > - * SPECIAL PROCEDURAL REQUIREMENTS > - * None > - * > - * INTERCASE DEPENDENCIES > - * None > - * > - * DETAILED DESCRIPTION > - * This is a Phase I test for the sync(2) system call. It is intended > - * to provide a limited exposure of the system call, for now. It > - * should/will be extended when full functional tests are written for > - * sync(2). > - * > - * Setup: > - * Setup signal handling. > - * Pause for SIGUSR1 if option specified. > - * > - * Test: > - * Loop if the proper options are given. > - * Execute system call > - * Check return code, if system call failed (return=-1) > - * Log the errno and Issue a FAIL message. > - * Otherwise, Issue a PASS message. > - * > - * Cleanup: > - * Print errno log and/or timing stats if options given > - * > - * > - *#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#**/ > - > -#include<errno.h> > -#include<string.h> > -#include<signal.h> > -#include "test.h" > - > -void setup(); > -void cleanup(); > - > -char *TCID = "sync01"; > -int TST_TOTAL = 1; > - > -int main(int ac, char **av) > -{ > - int lc; > - > - /*************************************************************** > - * parse standard options > - ***************************************************************/ > - tst_parse_opts(ac, av, NULL, NULL); > - > - /*************************************************************** > - * perform global setup for test > - ***************************************************************/ > - setup(); > - > - /*************************************************************** > - * check looping state if -c option given > - ***************************************************************/ > - for (lc = 0; TEST_LOOPING(lc); lc++) { > - > - tst_count = 0; > - > - /* > - * Call sync(2) > - */ > - TEST_VOID(sync()); > - > - /* check return code */ > - if (TEST_RETURN == -1) { > - tst_resm(TFAIL, "sync() Failed, errno=%d : %s", > - TEST_ERRNO, strerror(TEST_ERRNO)); > - } else { > - tst_resm(TPASS, "sync() returned %ld", > - TEST_RETURN); > - } > - } > - > - cleanup(); > - tst_exit(); > -} > - > -/*************************************************************** > - * setup() - performs all ONE TIME setup for this test. > - ***************************************************************/ > -void setup(void) > -{ > - > - tst_sig(NOFORK, DEF_HANDLER, cleanup); > - > - TEST_PAUSE; > - > -} > - > -/*************************************************************** > - * cleanup() - performs all ONE TIME cleanup for this test at > - * completion or premature exit. > - ***************************************************************/ > -void cleanup(void) > -{ > - > -}
Hi!
> Ping. I think this patchset is simple.
Great cleanup, pushed, thanks.
On 11/6/20 8:36 PM, Cyril Hrubis wrote: > Hi! >> Ping. I think this patchset is simple. > Great cleanup, pushed, thanks. Hi Cyril, Petr I have a doubt after reading Xu's patch[1] and Martin's patch[2]: 1) Xu removed sync01 because sync() always return 0. 2) On error, normal syscall always return -1 but Martin added a check for undefined return value(i.e. negative value except -1). Is it necessary to check undefined return value? Patches: [1]: [PATCH 1/4] syscalls/sync01: Remove it [2]: [PATCH 03/19] Unify error handling in lib/tst_safe_timerfd.c Best Regards, Xiao Yang >
Hi! > I have a doubt after reading Xu's patch[1] and Martin's patch[2]: > > 1) Xu removed sync01 because sync() always return 0. Actually sync() is defined as void function, so the tests were bogusly checking the TST_RET value which haven't been set at all.
On 11/7/20 12:47 AM, Cyril Hrubis wrote: > Hi! >> I have a doubt after reading Xu's patch[1] and Martin's patch[2]: >> >> 1) Xu removed sync01 because sync() always return 0. > Actually sync() is defined as void function, so the tests were bogusly > checking the TST_RET value which haven't been set at all. Hi Cyril, Oops, I gave a wrong example. :-( On error, I just wonder if we need to check all return value(i.e. negative value except -1). IOW, Is it possible for syscall to get a error value which is not -1? Best Regards, Xiao Yang >
Hi Xiao, Cyril > On 11/7/20 12:47 AM, Cyril Hrubis wrote: >> Hi! >>> I have a doubt after reading Xu's patch[1] and Martin's patch[2]: >>> >>> 1) Xu removed sync01 because sync() always return 0. >> Actually sync() is defined as void function, so the tests were bogusly >> checking the TST_RET value which haven't been set at all. > > Hi Cyril, > > Oops, I gave a wrong example. :-( > > On error, I just wonder if we need to check all return value(i.e. > negative value except -1). > > IOW, Is it possible for syscall to get a error value which is not -1? IMO, get a error and syscall return -1 that is a normal situation. Martin creates a standard model for it and doesn't match this rule is wrong, so we can check syscall whether return the right value when kernel changes these api in the future. Best Regards, Yang Xu > > Best Regards, > > Xiao Yang > >> > >
Hi, > On 11/7/20 12:47 AM, Cyril Hrubis wrote: > > Hi! > > > I have a doubt after reading Xu's patch[1] and Martin's patch[2]: > > > 1) Xu removed sync01 because sync() always return 0. > > Actually sync() is defined as void function, so the tests were bogusly > > checking the TST_RET value which haven't been set at all. > Hi Cyril, > Oops, I gave a wrong example. :-( > On error, I just wonder if we need to check all return value(i.e. negative > value except -1). > IOW, Is it possible for syscall to get a error value which is not -1? There are probably other examples, but I've found only these: man malloc_get_state(3) If the implementation detects that state does not point to a correctly formed data structure, malloc_set_state() returns -1. If the implementation detects that the version of the data structure referred to by state is a more recent version than this implementation knows about, malloc_set_state() returns -2. man mmap(2) On error, the value MAP_FAILED (that is, (void *) -1) is returned. > Best Regards, > Xiao Yang Kind regards, Petr
On 020/11/8 0:55, Petr Vorel wrote: > Hi, > >> On 11/7/20 12:47 AM, Cyril Hrubis wrote: >>> Hi! >>>> I have a doubt after reading Xu's patch[1] and Martin's patch[2]: >>>> 1) Xu removed sync01 because sync() always return 0. >>> Actually sync() is defined as void function, so the tests were bogusly >>> checking the TST_RET value which haven't been set at all. >> Hi Cyril, >> Oops, I gave a wrong example. :-( >> On error, I just wonder if we need to check all return value(i.e. negative >> value except -1). >> IOW, Is it possible for syscall to get a error value which is not -1? > There are probably other examples, but I've found only these: > > man malloc_get_state(3) > > If the implementation detects that state does not point to a correctly > formed data structure, malloc_set_state() returns -1. > If the implementation detects that the version of the data structure > referred to by state is a more recent version than this implementation knows > about, malloc_set_state() returns -2. > > man mmap(2) > On error, the value MAP_FAILED (that is, (void *) -1) is returned. Hi, Sorry, I didn't describe the doubt clearly. For example: 1) open(2) will return -1 if an error occur. Is it necessary to check invalid return value(except -1) if an error occur? 2) mmap(2) will return MAP_FAILED if an error occurs. Is it necessary to check invalid value(except MAP_FAILED) if an error occur? Martin's patches have added a check for invalid return value in many safe macros but a lot of syscall tests(e.g. after doingTEST()) don't add the check for now. I am not sure if we need to add the check for all syscall tests. :-) BTW: In my opinion, it is hardly to get invalid return value so the check seems unnecessary and redundance. Best Regards, Xiao Yang >> Best Regards, >> Xiao Yang > > Kind regards, > Petr >
Hi, > Sorry, I didn't describe the doubt clearly. > For example: > 1) open(2) will return -1 if an error occur. > Is it necessary to check invalid return value(except -1) if an error > occur? > 2) mmap(2) will return MAP_FAILED if an error occurs. > Is it necessary to check invalid value(except MAP_FAILED) if an error > occur? > Martin's patches have added a check for invalid return value in many safe > macros but a lot of syscall tests(e.g. after doingTEST()) don't add the > check for now. > I am not sure if we need to add the check for all syscall tests. :-) > BTW: In my opinion, it is hardly to get invalid return value so the check > seems unnecessary and redundance. Agree it's hard to get these errors. That's why I would bother just in the library (in these safe_*() functions). Kind regards, Petr Vorel > Best Regards, > Xiao Yang
Hi! > 1) open(2) will return -1 if an error occur. > Is it necessary to check invalid return value(except -1) if an > error occur? Well if there are values that are never supposed to be returned it makes sense to catch these and return a TBROK or TFAIL. If we are expecially testing a syscall() I would say that we should check for all kinds of errors including the values that shall not be returned e.g.: TEST(open(...)); if (TST_RET == -1) { tst_ret(TFAIL | TTERRNO, "open() failed"); return; } if (TST_RET < 0) { tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld", TST_RET); return; } ... If the syscall is part of the test preparation and there is no safe macro I would say that it's enough to cover all invalid values in one condition e.g.: fd = open(...); if (fd < 0) tst_brk(TBROK | TERRNO, "open() failed"); > 2) mmap(2) will return MAP_FAILED if an error occurs. > Is it necessary to check invalid value(except MAP_FAILED) if an > error occur? Actually return value from mmap() is pointer, right? And the only value that is not supposed to be returned is MAP_FAILED or do I miss something? > Martin's patches have added a check for invalid return value in many > safe macros but a lot of syscall tests(e.g. after doingTEST()) don't add > the check for now. > I am not sure if we need to add the check for all syscall tests. :-) I would say that at least for newly added test we should make sure that there is no unexpected value returned. > BTW: In my opinion, it is hardly to get invalid return value so the > check seems unnecessary and redundance. Well it's not a common case but I've seen this to happen a few times, once it was because a backported patch applied cleanly but the code was incorrect and as a result syscall started to return really unexpected values.
On Mon, 2020-11-09 at 13:42 +0100, Cyril Hrubis wrote: > Hi! > > 1) open(2) will return -1 if an error occur. > > Is it necessary to check invalid return value(except -1) if > > an > > error occur? > > Well if there are values that are never supposed to be returned it > makes > sense to catch these and return a TBROK or TFAIL. > > If we are expecially testing a syscall() I would say that we should > check for all kinds of errors including the values that shall not be > returned e.g.: > > TEST(open(...)); > > if (TST_RET == -1) { > tst_ret(TFAIL | TTERRNO, "open() failed"); > return; > } > > if (TST_RET < 0) { > tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld", > TST_RET); > return; > } > > ... I see no downside in checking for this unexpected negative value, except copy/pasting this test condition in every syscall testcase. I don't know the LTP codebase well enough yet, but what would you say is a good way to have this somewhere in the library. A TEST_SYSCALL macro, or something else, which fails if the return value is < -1?
Hi, > On Mon, 2020-11-09 at 13:42 +0100, Cyril Hrubis wrote: > > Hi! > > > 1) open(2) will return -1 if an error occur. > > > Is it necessary to check invalid return value(except -1) if > > > an > > > error occur? > > Well if there are values that are never supposed to be returned it > > makes > > sense to catch these and return a TBROK or TFAIL. > > If we are expecially testing a syscall() I would say that we should > > check for all kinds of errors including the values that shall not be > > returned e.g.: > > TEST(open(...)); > > if (TST_RET == -1) { > > tst_ret(TFAIL | TTERRNO, "open() failed"); > > return; > > } > > if (TST_RET < 0) { > > tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld", > > TST_RET); > > return; > > } > > ... > I see no downside in checking for this unexpected negative value, > except copy/pasting this test condition in every syscall testcase. > I don't know the LTP codebase well enough yet, but what would you say > is a good way to have this somewhere in the library. A TEST_SYSCALL > macro, or something else, which fails if the return value is < -1? LGTM. I was thinking about adding it directly into TEST() and define _TEST() which would not do that and be used in that few cases which ret < -1 is valid, but that would be ugly. Another candidate is macro for new API tst_syscall() defined in include/lapi/syscalls.h (generated in include/lapi/syscalls/regen.sh). Kind regards, Petr
Hi! > > I see no downside in checking for this unexpected negative value, > > except copy/pasting this test condition in every syscall testcase. > > > I don't know the LTP codebase well enough yet, but what would you say > > is a good way to have this somewhere in the library. A TEST_SYSCALL > > macro, or something else, which fails if the return value is < -1? > LGTM. I was thinking about adding it directly into TEST() and define _TEST() > which would not do that and be used in that few cases which ret < -1 is valid, > but that would be ugly. Well it would have to be a set of macros at least since: * There are different classes of functions by return values * We have possitive and negative testcases For example we would have to have two macros for functions that return file descriptors, one for a cases where we expect the function to return a valid file descriptor and one when we expect the function to fail. So it would look like: TEST_FD(open("/foo/bar", O_RDONLY)); or: TEST_FAIL(open((void*)-1, O_RDONLY)); The TEST_FD() macro would fail the test if the return value is < 0 And the TEST_FAIL() will fail the test unless we the return value is set to -1. Maybe we can even have a version with errno as well something as: TEST_FAIL_ERR(open((void*)-1, O_RDONLY), EFAULT);
diff --git a/runtest/syscalls b/runtest/syscalls index 0443f9f3d..2e7108655 100644 --- a/runtest/syscalls +++ b/runtest/syscalls @@ -1477,7 +1477,6 @@ symlink05 symlink05 #symlinkat test cases symlinkat01 symlinkat01 -sync01 sync01 sync02 sync02 sync03 sync03 diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore index 04f4710dd..d006746c2 100644 --- a/testcases/kernel/syscalls/sync/.gitignore +++ b/testcases/kernel/syscalls/sync/.gitignore @@ -1,3 +1,2 @@ -/sync01 /sync02 /sync03 diff --git a/testcases/kernel/syscalls/sync/sync01.c b/testcases/kernel/syscalls/sync/sync01.c deleted file mode 100644 index dd0a336c2..000000000 --- a/testcases/kernel/syscalls/sync/sync01.c +++ /dev/null @@ -1,182 +0,0 @@ -/* - * Copyright (c) 2000 Silicon Graphics, Inc. All Rights Reserved. - * - * 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. - * - * Further, this software is distributed without any warranty that it is - * free of the rightful claim of any third person regarding infringement - * or the like. Any license provided herein, whether implied or - * otherwise, applies only to this software file. Patent licenses, if - * any, provided herein do not apply to combinations of this program with - * other software, or any other product whatsoever. - * - * 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. - * - * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy, - * Mountain View, CA 94043, or: - * - * http://www.sgi.com - * - * For further information regarding this notice, see: - * - * http://oss.sgi.com/projects/GenInfo/NoticeExplan/ - * - */ -/* $Id: sync01.c,v 1.6 2009/11/02 13:57:19 subrata_modak Exp $ */ -/********************************************************** - * - * OS Test - Silicon Graphics, Inc. - * - * TEST IDENTIFIER : sync01 - * - * EXECUTED BY : anyone - * - * TEST TITLE : Basic test for sync(2) - * - * PARENT DOCUMENT : usctpl01 - * - * TEST CASE TOTAL : 1 - * - * WALL CLOCK TIME : 1 - * - * CPU TYPES : ALL - * - * AUTHOR : William Roske - * - * CO-PILOT : Dave Fenner - * - * DATE STARTED : 03/30/92 - * - * INITIAL RELEASE : UNICOS 7.0 - * - * TEST CASES - * - * 1.) sync(2) returns...(See Description) - * - * INPUT SPECIFICATIONS - * The standard options for system call tests are accepted. - * (See the parse_opts(3) man page). - * - * OUTPUT SPECIFICATIONS - * - * DURATION - * Terminates - with frequency and infinite modes. - * - * SIGNALS - * Uses SIGUSR1 to pause before test if option set. - * (See the parse_opts(3) man page). - * - * RESOURCES - * None - * - * ENVIRONMENTAL NEEDS - * No run-time environmental needs. - * - * SPECIAL PROCEDURAL REQUIREMENTS - * None - * - * INTERCASE DEPENDENCIES - * None - * - * DETAILED DESCRIPTION - * This is a Phase I test for the sync(2) system call. It is intended - * to provide a limited exposure of the system call, for now. It - * should/will be extended when full functional tests are written for - * sync(2). - * - * Setup: - * Setup signal handling. - * Pause for SIGUSR1 if option specified. - * - * Test: - * Loop if the proper options are given. - * Execute system call - * Check return code, if system call failed (return=-1) - * Log the errno and Issue a FAIL message. - * Otherwise, Issue a PASS message. - * - * Cleanup: - * Print errno log and/or timing stats if options given - * - * - *#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#**/ - -#include <errno.h> -#include <string.h> -#include <signal.h> -#include "test.h" - -void setup(); -void cleanup(); - -char *TCID = "sync01"; -int TST_TOTAL = 1; - -int main(int ac, char **av) -{ - int lc; - - /*************************************************************** - * parse standard options - ***************************************************************/ - tst_parse_opts(ac, av, NULL, NULL); - - /*************************************************************** - * perform global setup for test - ***************************************************************/ - setup(); - - /*************************************************************** - * check looping state if -c option given - ***************************************************************/ - for (lc = 0; TEST_LOOPING(lc); lc++) { - - tst_count = 0; - - /* - * Call sync(2) - */ - TEST_VOID(sync()); - - /* check return code */ - if (TEST_RETURN == -1) { - tst_resm(TFAIL, "sync() Failed, errno=%d : %s", - TEST_ERRNO, strerror(TEST_ERRNO)); - } else { - tst_resm(TPASS, "sync() returned %ld", - TEST_RETURN); - } - } - - cleanup(); - tst_exit(); -} - -/*************************************************************** - * setup() - performs all ONE TIME setup for this test. - ***************************************************************/ -void setup(void) -{ - - tst_sig(NOFORK, DEF_HANDLER, cleanup); - - TEST_PAUSE; - -} - -/*************************************************************** - * cleanup() - performs all ONE TIME cleanup for this test at - * completion or premature exit. - ***************************************************************/ -void cleanup(void) -{ - -}
This case tests whether sync() can return the correct value. But as man-page said "sync() is always successful". So this case is meaningless and remove it. Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- runtest/syscalls | 1 - testcases/kernel/syscalls/sync/.gitignore | 1 - testcases/kernel/syscalls/sync/sync01.c | 182 ---------------------- 3 files changed, 184 deletions(-) delete mode 100644 testcases/kernel/syscalls/sync/sync01.c