Message ID | 20191009061619.48677-2-lkml@jv-coder.de |
---|---|
State | Superseded |
Headers | show |
Series | [1/2] tst_test.sh: Add TST_USES_MODULE | expand |
Hi Joerg, > From: Joerg Vehlow <joerg.vehlow@aox-tech.de> > Adds a new library variable TST_USES_MODULE, that can be used, when a > test may need a module, but should not fail, if the module is not available. I wonder if TST_USES_MODULE is descriptive enough. But it looks to me better than TST_GET_MODPATH (which Cyril suggested in v3). We should think twice as _USES_ keyword should be used consistently for the same approach in different functionality (i.e. TST_USES_FOO is the same as TST_NEEDS_FOO, but not TCONF/TBROK if it fails). But whole concept of TST_USES_FOO looks to me a bit complicated, if needed only for modules. Cannot we just call _tst_find_module directly in this case and not introduce variable? ... > +++ b/doc/test-writing-guidelines.txt > @@ -2125,6 +2125,8 @@ simply by setting right '$TST_NEEDS_FOO'. > | 'TST_NEEDS_CMDS' | String with command names that has to be present for > the test (see below). > | 'TST_NEEDS_MODULE' | Test module name needed for the test (see below). > +| 'TST_USES_MODULE' | Same as TST_NEEDS_MODULE, except that a missing module > +| | is not an error. > | 'TST_NEEDS_DRIVERS'| Checks kernel drivers support for the test. > |============================================================================= > @@ -2174,7 +2176,7 @@ Locating kernel modules > +++++++++++++++++++++++ > The LTP build system can build kernel modules as well, setting > -'$TST_NEEDS_MODULE' to module name will cause to library to look for the > +'$TST_NEEDS_MODULE' to module name will cause the library to look for the This is unrelated change, I merged it as a separate commit (c518ee8b9). ... > +_tst_find_module() > +{ > + local _tst_module=$1 > + local _tst_is_required=${2:-0} > + > + for tst_module in "$_tst_module" \ > + "$LTPROOT/testcases/bin/$_tst_module" \ > + "$TST_STARTWD/$_tst_module"; do nit: (can be fixed by person who merges it): It's not visible, but uses more tags than it should be, so it looks like: + for tst_module in "$_tst_module" \ + "$LTPROOT/testcases/bin/$_tst_module" \ + "$TST_STARTWD/$_tst_module"; do + + if [ -f "$tst_module" ]; then + TST_MODPATH="$tst_module" + break + fi I actually like the original alignment created by Alexey: for tst_module in "$TST_NEEDS_MODULE" \ "$LTPROOT/testcases/bin/$TST_NEEDS_MODULE" \ "$TST_STARTWD/$TST_NEEDS_MODULE"; do > + > + if [ -f "$tst_module" ]; then > + TST_MODPATH="$tst_module" > + break > + fi > + done > + > + if [ -z "$TST_MODPATH" ]; then > + if [ $_tst_is_required -eq 1 ]; then > + tst_brk TCONF "Failed to find module '$_tst_module'" > + else > + tst_res TINFO "Module '$_tst_module' not found." nit: please drop dot at the end (can be fixed by person who merges it). > + fi > + else > + tst_res TINFO "Found module at '$TST_MODPATH'" > + fi nit: this is IMHO more readable if [ -n "$TST_MODPATH" ]; then tst_res TINFO "Found module at '$TST_MODPATH'" return fi if [ $_tst_is_required -eq 1 ]; then tst_brk TCONF "Failed to find module '$_tst_module'" else tst_res TINFO "Module '$_tst_module' not found" fi Kind regards, Petr
Hi, >> Adds a new library variable TST_USES_MODULE, that can be used, when a >> test may need a module, but should not fail, if the module is not available. > I wonder if TST_USES_MODULE is descriptive enough. But it looks to me better > than TST_GET_MODPATH (which Cyril suggested in v3). > > We should think twice as _USES_ keyword should be used consistently for the same > approach in different functionality (i.e. TST_USES_FOO is the same as > TST_NEEDS_FOO, but not TCONF/TBROK if it fails). > > But whole concept of TST_USES_FOO looks to me a bit complicated, if needed only > for modules. Cannot we just call _tst_find_module directly in this case and not > introduce variable? I was thinking about adding a function to search for a module, but struggled with returning the name of the found found module. I had something like MODPATH=$(tst_find_module "$MODULE_NAME") but this cannot use tst_res or tst_brk, which I don't like. I also don't like just calling _tst_find_module and getting the result in some "magic" variable. That is the reason why I went with Cyril's Idea of a variable > ... >> +_tst_find_module() >> +{ >> + local _tst_module=$1 >> + local _tst_is_required=${2:-0} >> + >> + for tst_module in "$_tst_module" \ >> + "$LTPROOT/testcases/bin/$_tst_module" \ >> + "$TST_STARTWD/$_tst_module"; do > nit: (can be fixed by person who merges it): It's not visible, but uses more > tags than it should be, so it looks like: > + for tst_module in "$_tst_module" \ > + "$LTPROOT/testcases/bin/$_tst_module" \ > + "$TST_STARTWD/$_tst_module"; do > + > + if [ -f "$tst_module" ]; then > + TST_MODPATH="$tst_module" > + break > + fi > I actually like the original alignment created by Alexey: > for tst_module in "$TST_NEEDS_MODULE" \ > "$LTPROOT/testcases/bin/$TST_NEEDS_MODULE" \ > "$TST_STARTWD/$TST_NEEDS_MODULE"; do Just an accident by my editor, I'll fix it for v2 > >> + >> + if [ -f "$tst_module" ]; then >> + TST_MODPATH="$tst_module" >> + break >> + fi >> + done >> + >> + if [ -z "$TST_MODPATH" ]; then >> + if [ $_tst_is_required -eq 1 ]; then >> + tst_brk TCONF "Failed to find module '$_tst_module'" >> + else >> + tst_res TINFO "Module '$_tst_module' not found." > nit: please drop dot at the end (can be fixed by person who merges it). Fixed for v2 >> + fi >> + else >> + tst_res TINFO "Found module at '$TST_MODPATH'" >> + fi > > nit: this is IMHO more readable > if [ -n "$TST_MODPATH" ]; then > tst_res TINFO "Found module at '$TST_MODPATH'" > return > fi > > if [ $_tst_is_required -eq 1 ]; then > tst_brk TCONF "Failed to find module '$_tst_module'" > else > tst_res TINFO "Module '$_tst_module' not found" > fi It would still keep the else and not use a return. Indentation clearly shows what's going on. But I agree to invert the logic, first testing the good case, than the bad. > > Kind regards, > Petr Jörg
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index cd0d28b8e..4a0652a8d 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -2125,6 +2125,8 @@ simply by setting right '$TST_NEEDS_FOO'. | 'TST_NEEDS_CMDS' | String with command names that has to be present for the test (see below). | 'TST_NEEDS_MODULE' | Test module name needed for the test (see below). +| 'TST_USES_MODULE' | Same as TST_NEEDS_MODULE, except that a missing module +| | is not an error. | 'TST_NEEDS_DRIVERS'| Checks kernel drivers support for the test. |============================================================================= @@ -2174,7 +2176,7 @@ Locating kernel modules +++++++++++++++++++++++ The LTP build system can build kernel modules as well, setting -'$TST_NEEDS_MODULE' to module name will cause to library to look for the +'$TST_NEEDS_MODULE' to module name will cause the library to look for the module in a few possible paths. If module was found the path to it will be stored into '$TST_MODPATH' diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index e0b24c6b9..c70a5abbe 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -396,6 +396,32 @@ _tst_require_root() fi } +_tst_find_module() +{ + local _tst_module=$1 + local _tst_is_required=${2:-0} + + for tst_module in "$_tst_module" \ + "$LTPROOT/testcases/bin/$_tst_module" \ + "$TST_STARTWD/$_tst_module"; do + + if [ -f "$tst_module" ]; then + TST_MODPATH="$tst_module" + break + fi + done + + if [ -z "$TST_MODPATH" ]; then + if [ $_tst_is_required -eq 1 ]; then + tst_brk TCONF "Failed to find module '$_tst_module'" + else + tst_res TINFO "Module '$_tst_module' not found." + fi + else + tst_res TINFO "Found module at '$TST_MODPATH'" + fi +} + tst_run() { local _tst_i @@ -410,7 +436,7 @@ tst_run() SETUP|CLEANUP|TESTFUNC|ID|CNT|MIN_KVER);; OPTS|USAGE|PARSE_ARGS|POS_ARGS);; NEEDS_ROOT|NEEDS_TMPDIR|TMPDIR|NEEDS_DEVICE|DEVICE);; - NEEDS_CMDS|NEEDS_MODULE|MODPATH|DATAROOT);; + NEEDS_CMDS|NEEDS_MODULE|USES_MODULE|MODPATH|DATAROOT);; NEEDS_DRIVERS|FS_TYPE|MNTPOINT|MNT_PARAMS);; IPV6|IPVER|TEST_DATA|TEST_DATA_IFS);; RETRY_FUNC|RETRY_FN_EXP_BACKOFF);; @@ -487,22 +513,12 @@ tst_run() TST_DEVICE_FLAG=1 fi - if [ -n "$TST_NEEDS_MODULE" ]; then - for tst_module in "$TST_NEEDS_MODULE" \ - "$LTPROOT/testcases/bin/$TST_NEEDS_MODULE" \ - "$TST_STARTWD/$TST_NEEDS_MODULE"; do - - if [ -f "$tst_module" ]; then - TST_MODPATH="$tst_module" - break - fi - done - - if [ -z "$TST_MODPATH" ]; then - tst_brk TCONF "Failed to find module '$TST_NEEDS_MODULE'" - else - tst_res TINFO "Found module at '$TST_MODPATH'" - fi + if [ -n "$TST_NEEDS_MODULE" ] && [ -n "$TST_USES_MODULE" ]; then + tst_brk TBROK "Setting TST_NEEDS_MODULE and TST_USES_MODULE at the same time is not allowed" + elif [ -n "$TST_NEEDS_MODULE" ]; then + _tst_find_module "$TST_NEEDS_MODULE" 1 + elif [ -n "$TST_USES_MODULE" ]; then + _tst_find_module "$TST_USES_MODULE" 0 fi if [ -n "$TST_SETUP" ]; then
From: Joerg Vehlow <joerg.vehlow@aox-tech.de> Adds a new library variable TST_USES_MODULE, that can be used, when a test may need a module, but should not fail, if the module is not available. --- doc/test-writing-guidelines.txt | 4 ++- testcases/lib/tst_test.sh | 50 ++++++++++++++++++++++----------- 2 files changed, 36 insertions(+), 18 deletions(-)