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

Petr Vorel
2023-11-03T12:12:20Z

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);
> > + }
> > + }