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

[Petr Vorel <pvorel@suse.cz>]

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

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