Re: [LTP] [PATCH 2/4] lib: Add .modprobe

Cyril Hrubis
2023-11-01T16:26:04Z

Hi!
> diff --git a/doc/C-Test-API.asciidoc b/doc/C-Test-API.asciidoc
> index dab811564..f2ba302e2 100644
> --- a/doc/C-Test-API.asciidoc
> +++ b/doc/C-Test-API.asciidoc
> @@ -1609,6 +1609,11 @@ first missing driver.
> The detection is based on reading 'modules.dep' and 'modules.builtin' files
> generated by kmod. The check is skipped on Android.
>
> +NULL terminated array '.modprobe' of kernel module names are tried to be loaded
^
attempted
> +with 'modprobe' unless they are builtin or already loaded. Test exits with
> +'TCONF' on first 'modprobe' non-zero exit. During cleanup are the modules
^ ^
failure
> +loaded by the test unloaded with 'rmmod'.

During the cleanup modules that have been loaded are unloaded by 'modprobe -r'.

> 1.27 Saving & restoring /proc|sys values
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> diff --git a/doc/Test-Writing-Guidelines.asciidoc b/doc/Test-Writing-Guidelines.asciidoc
> index 0db852ae6..19487816e 100644
> --- a/doc/Test-Writing-Guidelines.asciidoc
> +++ b/doc/Test-Writing-Guidelines.asciidoc
> @@ -371,6 +371,7 @@ https://github.com/linux-test-project/ltp/wiki/Shell-Test-API[Shell Test API].
> | '.min_mem_avail' | not applicable
> | '.mnt_flags' | 'TST_MNT_PARAMS'
> | '.min_swap_avail' | not applicable
> +| '.modprobe' | –
> | '.mntpoint', '.mnt_data' | 'TST_MNTPOINT'
> | '.mount_device' | 'TST_MOUNT_DEVICE'
> | '.needs_cgroup_ctrls' | –
> diff --git a/include/tst_test.h b/include/tst_test.h
> index 75c2109b9..6b4fac985 100644
> --- a/include/tst_test.h
> +++ b/include/tst_test.h
> @@ -297,9 +297,12 @@ struct tst_test {
> /* NULL terminated array of resource file names */
> const char *const *resource_files;
>
> - /* NULL terminated array of needed kernel drivers */
> + /* NULL terminated array of needed kernel drivers to be checked */
> const char * const *needs_drivers;

Do we need this array? Are there tests that needs to check for a module
but does not want the library to load them?

> + /* NULL terminated array of needed kernel drivers to be loaded with modprobe */
> + const char * const *modprobe;
> +
> /*
> * {NULL, NULL} terminated array of (/proc, /sys) files to save
> * before setup and restore after cleanup
> diff --git a/lib/tst_test.c b/lib/tst_test.c
> index 087c62a16..ccbaa4c02 100644
> --- a/lib/tst_test.c
> +++ b/lib/tst_test.c
> @@ -49,6 +49,7 @@ const char *TCID __attribute__((weak));
> #define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
>
> #define DEFAULT_TIMEOUT 30
> +#define MODULES_MAX_LEN 10
>
> struct tst_test *tst_test;
>
> @@ -83,6 +84,8 @@ const char *tst_ipc_path = ipc_path;
>
> static char shm_path[1024];
>
> +static int modules_loaded[MODULES_MAX_LEN];
> +
> int TST_ERR;
> int TST_PASS;
> long TST_RET;
> @@ -1135,6 +1138,29 @@ static void do_cgroup_requires(void)
> tst_cg_init();
> }
>
> +/*
> + * Search kernel driver in /proc/modules.
> + *
> + * @param driver The name of the driver.
> + * @return 1 if driver is found, otherwise 0.
> + */
> +static int module_loaded(const char *driver)
> +{
> + char line[4096];
> + int found = 0;
> + FILE *file = SAFE_FOPEN("/proc/modules", "r");
> +
> + while (fgets(line, sizeof(line), file)) {
> + if (strstr(line, driver)) {

Is this realy okay? What if a module name is a substring of other
module? We have module names that ar as short as two letters, for
instance with 'sg' or 'ac' there quite a bit of room for error.

We really need to find first whitespace in the line and replace it with
'\0' then do strcmp().

And we have to do the '_' and '-' permutations as well, as we do in the
code that checks for buildin modules.

Maybe we need module_strcmp(), that would work like strcmp() but would
handle the '-' and '_' as the same letter.

> + found = 1;
> + break;
> + }
> + }
> + SAFE_FCLOSE(file);
> +
> + return found;
> +}
> +
> static void do_setup(int argc, char *argv[])
> {
> if (!tst_test)
> @@ -1194,6 +1220,20 @@ static void do_setup(int argc, char *argv[])
> safe_check_driver(name);
> }
>
> + if (tst_test->modprobe) {
> + const char *name;
> + int i;
> +
> + for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> + /* only load module if not already loaded */
> + if (!module_loaded(name) && tst_check_builtin_driver(name)) {
> + const char *const cmd_modprobe[] = {"modprobe", name, NULL};
> + SAFE_CMD(cmd_modprobe, NULL, NULL);

We are supposed to TCONF here if modprobe failed, so we have to check
the return value and tst_brk(TCONF, "...") when it's non-zero, right?

> + modules_loaded[i] = 1;
> + }
> + }
> + }
> +
> if (tst_test->mount_device)
> tst_test->format_device = 1;
>
> @@ -1362,6 +1402,19 @@ static void do_cleanup(void)
>
> tst_sys_conf_restore(0);
>
> + if (tst_test->modprobe) {
> + const char *name;
> + int i;
> +
> + for (i = 0; (name = tst_test->modprobe[i]); ++i) {
> + if (!modules_loaded[i])
> + continue;
> +
> + const char *const cmd_rmmod[] = {"rmmod", name, NULL};

modprobe -r please, rmmod has been deprecated for ages.

> + SAFE_CMD(cmd_rmmod, NULL, NULL);
> + }
> + }
> +
> if (tst_test->restore_wallclock)
> tst_wallclock_restore();
>
> --
> 2.42.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp

--
Cyril Hrubis
chrubis@suse.cz