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

[Petr Vorel <pvorel@suse.cz>]

Hi Cyril,

thanks for your comments. More questions bellow.

...
> > +++ 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'.

Thanks for the wording improvement. I also agree that 'modprobe -r' is probably
better than 'rmmod'. But we already have tst_module_unload_() in lib/tst_module.c
(file for both APIs). I guess I'll add new functions to lib/tst_kernel.c, which
is new API only and already has some module specific files (not ideal name, but
after everything using lib/tst_module.c is converted to the new API we can move
module specific files to lib/tst_module.c).

...
> > +++ 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?

So you would, as a part of this change, replace .needs_drivers with .modprobe_module.

I'm not sure if it's a good idea. Some kernel modules are loaded on demand. If
we call modprobe, we will skip testing of this auto-load functionality.
What come to my mind are various modules required by certain socket() call, see
bind0[45].c., but they don't use .needs_drivers.

Other example is loop module in .needs_drivers (e.g. ioctl/ioctl_loop05.c and
others) which load loop module via ioctl(fd, LOOP_CONFIGURE, ...) or quotactl
tests.

IMHO zram03.c cannot just load module. zram-control hot_add/hot_remove
functionality was added in 6566d1a32bf7 ("zram: add dynamic device add/remove
functionality") in v4.2-rc1 - still too new to drop the support.
I still believe we can start with modprobe via .modprobe_module for zram03.c,
and then do the other checks (I'll try to send patch first with Cc: Yang Xu).

> > +	/* NULL terminated array of needed kernel drivers to be loaded with modprobe */
> > +	const char * const *modprobe;
...

> > +/*
> > + * 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().

+1

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

Yep, that part has been solved in tst_search_driver_(), which is in
lib/tst_kernel.c, but that function is searching in /lib/modules.

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

+1, it will be then used in two places.

> > +			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?

Yes, but see safe_cmd() in lib/tst_safe_macros.c. tst_cmd() is called with 
TST_CMD_PASS_RETVAL | TST_CMD_TCONF_ON_MISSING, thus this is covered.
I agree SAFE_CMD() is confusing, I'll send a patch documenting this in
include/tst_safe_macros.h.

> > +				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.

Ah, here goes the reason. Should it be replaced also in tst_module_unload_()?

Kind regards,
Petr

> > +			SAFE_CMD(cmd_rmmod, NULL, NULL);
> > +		}
> > +	}

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