[1/2] tst_test.sh: Add TST_USES_MODULE
diff mbox series

Message ID 20191009061619.48677-2-lkml@jv-coder.de
State New
Headers show
Series
  • [1/2] tst_test.sh: Add TST_USES_MODULE
Related show

Commit Message

Joerg Vehlow Oct. 9, 2019, 6:16 a.m. UTC
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(-)

Comments

Petr Vorel Oct. 9, 2019, 7:36 a.m. UTC | #1
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
Joerg Vehlow Oct. 9, 2019, 7:48 a.m. UTC | #2
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

Patch
diff mbox series

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