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

[Cyril Hrubis <chrubis@suse.cz>]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp