[v2,1/4] lib/tst_test.c: add 'needs_drivers' option with tst_check_drivers cmd
diff mbox series

Message ID 1534764229-26993-1-git-send-email-alexey.kodanev@oracle.com
State Superseded
Delegated to: Petr Vorel
Headers show
Series
  • [v2,1/4] lib/tst_test.c: add 'needs_drivers' option with tst_check_drivers cmd
Related show

Commit Message

Alexey Kodanev Aug. 20, 2018, 11:23 a.m. UTC
The drivers are checked with modprobe. If modprobe is not available
on the system, the checks are silently skipped.

Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
v2: * moved tst_check_driver() from tst_test.h to tst_kernel.h
    * added the new option description to the doc and comment to tst_kernel.h
    * iterating over the driver list moved out from tst_check_drivers(), the
      function renamed accordingly.

 doc/test-writing-guidelines.txt   |   10 ++++++++++
 include/tst_kernel.h              |    8 ++++++++
 include/tst_test.h                |    3 +++
 lib/tst_kernel.c                  |    9 +++++++++
 lib/tst_test.c                    |    9 +++++++++
 testcases/lib/.gitignore          |    1 +
 testcases/lib/Makefile            |    2 +-
 testcases/lib/tst_check_drivers.c |   24 ++++++++++++++++++++++++
 8 files changed, 65 insertions(+), 1 deletions(-)
 create mode 100644 testcases/lib/tst_check_drivers.c

Comments

Petr Vorel Aug. 22, 2018, 3:41 p.m. UTC | #1
Hi Alexey,

> The drivers are checked with modprobe. If modprobe is not available
> on the system, the checks are silently skipped.

> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> ---
> v2: * moved tst_check_driver() from tst_test.h to tst_kernel.h
>     * added the new option description to the doc and comment to tst_kernel.h
>     * iterating over the driver list moved out from tst_check_drivers(), the
>       function renamed accordingly.

...
> +2.2.26 Checking kernel for the driver support
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> + Someties test needs certain drivers to be available in the kernel and must
Typo Sometimes

> +end with TCONF if any are missing. For this task there is the '.needs_drivers'
> +option which accepts NULL-terminated array of the drivers names.
> +
> +Since it relies on modprobe command, the check will be skipped if the command
> +itself is not available on the system.


> +++ b/testcases/lib/tst_check_drivers.c
...
> +	if (argc < 2) {
> +		fprintf(stderr, "Please provide kernel driver list\n");
> +		return 1;
> +	}
> +
> +	for (i = 1; (name = argv[i]); ++i)
> +		if (tst_check_driver(name)) {
> +			fprintf(stderr, "%s", name);
> +			return 1;
> +		}
> +
> +	return 0;
> +}
It'd be handy to be able to load module with parameters. E.g.:
tcrypt sec=1 mode=200

We have 2 options: to name strig by spaces to b:
./tst_check_drivers "tcrypt sec=1 mode=200" other_module

Or handle in tst_check_drivers.c just one module and call it several times.

None of them is elegant.

Otherwise LGTM.


Kind regards,
Petr
Alexey Kodanev Aug. 27, 2018, 10:44 a.m. UTC | #2
On 08/22/2018 06:41 PM, Petr Vorel wrote:
> Hi Alexey,
> 
>> The drivers are checked with modprobe. If modprobe is not available
>> on the system, the checks are silently skipped.
> 
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 

Hi Petr,

>> ---
>> v2: * moved tst_check_driver() from tst_test.h to tst_kernel.h
>>     * added the new option description to the doc and comment to tst_kernel.h
>>     * iterating over the driver list moved out from tst_check_drivers(), the
>>       function renamed accordingly.
> 
> ...
>> +2.2.26 Checking kernel for the driver support
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> + Someties test needs certain drivers to be available in the kernel and must
> Typo Sometimes
> 
>> +end with TCONF if any are missing. For this task there is the '.needs_drivers'
>> +option which accepts NULL-terminated array of the drivers names.
>> +
>> +Since it relies on modprobe command, the check will be skipped if the command
>> +itself is not available on the system.
> 
> 
>> +++ b/testcases/lib/tst_check_drivers.c
> ...
>> +	if (argc < 2) {
>> +		fprintf(stderr, "Please provide kernel driver list\n");
>> +		return 1;
>> +	}
>> +
>> +	for (i = 1; (name = argv[i]); ++i)
>> +		if (tst_check_driver(name)) {
>> +			fprintf(stderr, "%s", name);
>> +			return 1;
>> +		}
>> +
>> +	return 0;
>> +}
> It'd be handy to be able to load module with parameters. E.g.:
> tcrypt sec=1 mode=200
> 
Do you want to start tcrypt module in the test setup? It's probably
better to add the -n/--dry-run option, so at first we verify that
the kernel actually has it, then either end with TCONF or continue.
That's the purpose of the 'needs_drivers'.

Moreover, if the failure occurs during the loading instead, the test
must return TBROK, not TCONF, right?

> We have 2 options: to name strig by spaces to b:
> ./tst_check_drivers "tcrypt sec=1 mode=200" other_module
> 
> Or handle in tst_check_drivers.c just one module and call it several times.

like modprobe?

> 
> None of them is elegant.
> 
> Otherwise LGTM.
> 
> 
> Kind regards,
> Petr
>
Petr Vorel Sept. 6, 2018, 5:53 p.m. UTC | #3
Hi Alexey,

> The drivers are checked with modprobe. If modprobe is not available
> on the system, the checks are silently skipped.

> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> v2: * moved tst_check_driver() from tst_test.h to tst_kernel.h
>     * added the new option description to the doc and comment to tst_kernel.h
>     * iterating over the driver list moved out from tst_check_drivers(), the
>       function renamed accordingly.

>  doc/test-writing-guidelines.txt   |   10 ++++++++++
>  include/tst_kernel.h              |    8 ++++++++
>  include/tst_test.h                |    3 +++
>  lib/tst_kernel.c                  |    9 +++++++++
>  lib/tst_test.c                    |    9 +++++++++
>  testcases/lib/.gitignore          |    1 +
>  testcases/lib/Makefile            |    2 +-
>  testcases/lib/tst_check_drivers.c |   24 ++++++++++++++++++++++++
>  8 files changed, 65 insertions(+), 1 deletions(-)
>  create mode 100644 testcases/lib/tst_check_drivers.c

I'd like this whole patchset being merged before git freeze.
Do you plan it?


Kind regards,
Petr
Cyril Hrubis Sept. 13, 2018, 12:27 p.m. UTC | #4
Hi!
> +2.2.26 Checking kernel for the driver support
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Someties test needs certain drivers to be available in the kernel and must
> +end with TCONF if any are missing. For this task there is the '.needs_drivers'
> +option which accepts NULL-terminated array of the drivers names.

Maybe we should explicitly say here that this works for both drivers
that are compiled-in and modules, since otherwise this may confuse the
users, especially since we talk about modprobe in the next paragraph.

I would write something as:

"Some tests may need specific kernel drivers, either compiled in, or
loaded as a module. If .need_drivers points to a NULL-terminated array
of kernel module names these are all checked and the test exits with
TCONF on first missing driver."

> +Since it relies on modprobe command, the check will be skipped if the command
> +itself is not available on the system.
> +
>  2.3 Writing a testcase in shell
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/include/tst_kernel.h b/include/tst_kernel.h
> index 5d5c04c..88941e1 100644
> --- a/include/tst_kernel.h
> +++ b/include/tst_kernel.h
> @@ -23,4 +23,12 @@
>   */
>  int tst_kernel_bits(void);
>  
> +/**
> + * Checks support for the kernel driver.
> + *
> + * @param name The name of the driver.
> + * @return Returns 0 if the kernel has the driver or modprobe is missing.
> + */
> +int tst_check_driver(const char *name);
> +
>  #endif	/* TST_KERNEL_H__ */
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 98dacf3..55c1418 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -170,6 +170,9 @@ struct tst_test {
>  
>  	/* NULL terminated array of resource file names */
>  	const char *const *resource_files;
> +
> +	/* NULL terminated array of needed kernel drivers */
> +	const char * const *needs_drivers;
>  };
>  
>  /*
> diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
> index 42d64cb..ad00d2d 100644
> --- a/lib/tst_kernel.c
> +++ b/lib/tst_kernel.c
> @@ -45,3 +45,12 @@ int tst_kernel_bits(void)
>  
>  	return kernel_bits;
>  }
> +
> +int tst_check_driver(const char *name)
> +{
> +	const char * const argv[] = { "modprobe", name, NULL };
> +	int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null", 1);
> +
> +	/* 255 - it looks like modprobe not available */
> +	return (res == 255) ? 0 : res;
> +}
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 2f3d357..dcc4088 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -767,6 +767,15 @@ static void do_setup(int argc, char *argv[])
>  	if (tst_test->min_kver)
>  		check_kver();
>  
> +	if (tst_test->needs_drivers) {
> +		const char *name;
> +		int i;
> +
> +		for (i = 0; (name = tst_test->needs_drivers[i]); ++i)
> +			if (tst_check_driver(name))
> +				tst_brk(TCONF, "%s driver not available", name);
> +	}
> +
>  	if (tst_test->format_device)
>  		tst_test->needs_device = 1;
>  
> diff --git a/testcases/lib/.gitignore b/testcases/lib/.gitignore
> index a9034e4..d83a48e 100644
> --- a/testcases/lib/.gitignore
> +++ b/testcases/lib/.gitignore
> @@ -1,5 +1,6 @@
>  /tst_sleep
>  /tst_random
> +/tst_check_drivers
>  /tst_checkpoint
>  /tst_rod
>  /tst_kvcmp
> diff --git a/testcases/lib/Makefile b/testcases/lib/Makefile
> index 3547e16..e1dea3b 100644
> --- a/testcases/lib/Makefile
> +++ b/testcases/lib/Makefile
> @@ -28,6 +28,6 @@ INSTALL_TARGETS		:= *.sh
>  
>  MAKE_TARGETS		:= tst_sleep tst_random tst_checkpoint tst_rod tst_kvcmp\
>  			   tst_device tst_net_iface_prefix tst_net_ip_prefix tst_net_vars\
> -			   tst_getconf tst_supported_fs
> +			   tst_getconf tst_supported_fs tst_check_drivers
>  
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/lib/tst_check_drivers.c b/testcases/lib/tst_check_drivers.c
> new file mode 100644
> index 0000000..c1d879d
> --- /dev/null
> +++ b/testcases/lib/tst_check_drivers.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (c) 2018 Oracle and/or its affiliates. All Rights Reserved. */
> +
> +#include <stdio.h>
> +#include "tst_kernel.h"
> +
> +int main(int argc, const char *argv[])
> +{
> +	const char *name;
> +	int i;
> +
> +	if (argc < 2) {
> +		fprintf(stderr, "Please provide kernel driver list\n");
> +		return 1;
> +	}
> +
> +	for (i = 1; (name = argv[i]); ++i)
> +		if (tst_check_driver(name)) {
> +			fprintf(stderr, "%s", name);
> +			return 1;
> +		}

I would be happier if we put a curly braces around large blocks inside
of other blocks as LKML prefers to do, since this piece of code is
starting to look a bit like python...


Other than that this looks very good.
Alexey Kodanev Sept. 14, 2018, 11:41 a.m. UTC | #5
On 09/13/2018 03:27 PM, Cyril Hrubis wrote:
> Hi!
>> +2.2.26 Checking kernel for the driver support
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Someties test needs certain drivers to be available in the kernel and must
>> +end with TCONF if any are missing. For this task there is the '.needs_drivers'
>> +option which accepts NULL-terminated array of the drivers names.
> 
> Maybe we should explicitly say here that this works for both drivers
> that are compiled-in and modules, since otherwise this may confuse the
> users, especially since we talk about modprobe in the next paragraph.
> 
> I would write something as:
> 
> "Some tests may need specific kernel drivers, either compiled in, or
> loaded as a module. If .need_drivers points to a NULL-terminated array
> of kernel module names these are all checked and the test exits with
> TCONF on first missing driver."
> 

OK, thanks! I guess this also correlates with using the --dry-run option
of modprobe that I'm going to add in the 3rd version [1], if there are no
objections.

[1] http://lists.linux.it/pipermail/ltp/2018-August/009128.html

Patch
diff mbox series

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index a169724..7a458d5 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -1449,6 +1449,16 @@  https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
 CRC32c checksum generation is supported by LTP. In order to use it, the
 test should include "tst_checksum.h" header, then can call tst_crc32c().
 
+2.2.26 Checking kernel for the driver support
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Someties test needs certain drivers to be available in the kernel and must
+end with TCONF if any are missing. For this task there is the '.needs_drivers'
+option which accepts NULL-terminated array of the drivers names.
+
+Since it relies on modprobe command, the check will be skipped if the command
+itself is not available on the system.
+
 2.3 Writing a testcase in shell
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/include/tst_kernel.h b/include/tst_kernel.h
index 5d5c04c..88941e1 100644
--- a/include/tst_kernel.h
+++ b/include/tst_kernel.h
@@ -23,4 +23,12 @@ 
  */
 int tst_kernel_bits(void);
 
+/**
+ * Checks support for the kernel driver.
+ *
+ * @param name The name of the driver.
+ * @return Returns 0 if the kernel has the driver or modprobe is missing.
+ */
+int tst_check_driver(const char *name);
+
 #endif	/* TST_KERNEL_H__ */
diff --git a/include/tst_test.h b/include/tst_test.h
index 98dacf3..55c1418 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -170,6 +170,9 @@  struct tst_test {
 
 	/* NULL terminated array of resource file names */
 	const char *const *resource_files;
+
+	/* NULL terminated array of needed kernel drivers */
+	const char * const *needs_drivers;
 };
 
 /*
diff --git a/lib/tst_kernel.c b/lib/tst_kernel.c
index 42d64cb..ad00d2d 100644
--- a/lib/tst_kernel.c
+++ b/lib/tst_kernel.c
@@ -45,3 +45,12 @@  int tst_kernel_bits(void)
 
 	return kernel_bits;
 }
+
+int tst_check_driver(const char *name)
+{
+	const char * const argv[] = { "modprobe", name, NULL };
+	int res = tst_run_cmd_(NULL, argv, "/dev/null", "/dev/null", 1);
+
+	/* 255 - it looks like modprobe not available */
+	return (res == 255) ? 0 : res;
+}
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2f3d357..dcc4088 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -767,6 +767,15 @@  static void do_setup(int argc, char *argv[])
 	if (tst_test->min_kver)
 		check_kver();
 
+	if (tst_test->needs_drivers) {
+		const char *name;
+		int i;
+
+		for (i = 0; (name = tst_test->needs_drivers[i]); ++i)
+			if (tst_check_driver(name))
+				tst_brk(TCONF, "%s driver not available", name);
+	}
+
 	if (tst_test->format_device)
 		tst_test->needs_device = 1;
 
diff --git a/testcases/lib/.gitignore b/testcases/lib/.gitignore
index a9034e4..d83a48e 100644
--- a/testcases/lib/.gitignore
+++ b/testcases/lib/.gitignore
@@ -1,5 +1,6 @@ 
 /tst_sleep
 /tst_random
+/tst_check_drivers
 /tst_checkpoint
 /tst_rod
 /tst_kvcmp
diff --git a/testcases/lib/Makefile b/testcases/lib/Makefile
index 3547e16..e1dea3b 100644
--- a/testcases/lib/Makefile
+++ b/testcases/lib/Makefile
@@ -28,6 +28,6 @@  INSTALL_TARGETS		:= *.sh
 
 MAKE_TARGETS		:= tst_sleep tst_random tst_checkpoint tst_rod tst_kvcmp\
 			   tst_device tst_net_iface_prefix tst_net_ip_prefix tst_net_vars\
-			   tst_getconf tst_supported_fs
+			   tst_getconf tst_supported_fs tst_check_drivers
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/lib/tst_check_drivers.c b/testcases/lib/tst_check_drivers.c
new file mode 100644
index 0000000..c1d879d
--- /dev/null
+++ b/testcases/lib/tst_check_drivers.c
@@ -0,0 +1,24 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright (c) 2018 Oracle and/or its affiliates. All Rights Reserved. */
+
+#include <stdio.h>
+#include "tst_kernel.h"
+
+int main(int argc, const char *argv[])
+{
+	const char *name;
+	int i;
+
+	if (argc < 2) {
+		fprintf(stderr, "Please provide kernel driver list\n");
+		return 1;
+	}
+
+	for (i = 1; (name = argv[i]); ++i)
+		if (tst_check_driver(name)) {
+			fprintf(stderr, "%s", name);
+			return 1;
+		}
+
+	return 0;
+}