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

Petr Vorel
2023-10-27T12:01:20Z

Hi all,

[ Cc Cyril ]

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

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

> What about renaming it as tst_module_is_loaded() and move into tst_kernel.h?
> I guess we could make use of it widely for checking module loading or not.

I can do that, but lib/tst_kernel.c uses the old API. I guess it would fit
better in lib/tst_module.c, but that also uses the old API. Most of the tests
are converted, but at least these modules are still in the old API and use
tst_module_load from tst_module.h:

testcases/kernel/device-drivers/acpi/ltp_acpi.c
testcases/kernel/device-drivers/block/block_dev_user/block_dev.c
testcases/kernel/device-drivers/pci/tpci_user/tpci.c
testcases/kernel/device-drivers/uaccess/uaccess.c
testcases/kernel/firmware/fw_load_user/fw_load.c
testcases/kernel/device-drivers/tbio/tbio_user/tbio.c

All but the last one were written by Alexey, they look ok, so they should
probably be converted. But I would rather not block this .modprobe_module
effort due this.

IMHO We need another file, which would be new API only. I'm also not sure if
it's a good idea to put another file with just single function to it. We already
have 38 lib/tst_*.c files which use new API. Any tip, what to use?
Or should I really put it into lib/tst_module.c ain include/tst_module.h, but
not into include/old/old_module.h (as we want old tests to be converted first?).

> > +{
> > + char line[4096];
> > + int found = 0;
> > + FILE *file = SAFE_FOPEN("/proc/modules", "r");
> > +
> > + while (fgets(line, sizeof(line), file)) {
> > + if (strstr(line, driver)) {
> > + 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);

> And here print the name to tell people the module is loaded.

+1

> > + modules_loaded[i] = 1;
> > + }
> > + }
> > + }

> This part could be as a separate function like tst_load_module() and
> built single into another lib. We prefer to keep the main tst_test.c
> as a simple outline.

+1 for a separate function, it should be in the same file as
tst_module_is_loaded().

> On the other hand, the extern functions can be used separately to let
> modules to be loaded and unloaded during the test iteration.
> It gives us more flexibility in test case design.

Having it as the separate function would allow to use it in
kvm_pagefault01.c and zram03.c - tiny simplification as they now call
SAFE_CMD().

kvm_pagefault01.c and can_common.h use them parameters, it might be worth
to implement them.

> > +
> > 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};
> > + SAFE_CMD(cmd_rmmod, NULL, NULL);

> Print unload module name.

+1

> > + }
> > + }

> Here as well. something maybe like tst_unload_module().

+1

Kind regards,
Petr