[LTP] [PATCH] block_dev: Convert to new API
Ricardo B. Marliere
rbm@suse.com
Mon Oct 21 21:40:10 CEST 2024
Hi Cyril,
On 21 Oct 24 17:24, Cyril Hrubis wrote:
> Hi!
> > .../block/block_dev_kernel/ltp_block_dev.c | 1 -
> > .../block/block_dev_user/block_dev.c | 78 +++++++++-------------
> > 2 files changed, 32 insertions(+), 47 deletions(-)
> >
> > diff --git a/testcases/kernel/device-drivers/block/block_dev_kernel/ltp_block_dev.c b/testcases/kernel/device-drivers/block/block_dev_kernel/ltp_block_dev.c
> > index 17047c0d5ae3f6556f3fa4b0eb2a17a86e5f05a6..8a3ee4ff0bac6ed46c7c09c5752b6fec3ef01a40 100644
> > --- a/testcases/kernel/device-drivers/block/block_dev_kernel/ltp_block_dev.c
> > +++ b/testcases/kernel/device-drivers/block/block_dev_kernel/ltp_block_dev.c
> > @@ -12,7 +12,6 @@
> > #include <linux/module.h>
> > #include <linux/device.h>
> > #include <linux/fs.h>
> > -#include <linux/genhd.h>
> > #include <linux/blkdev.h>
>
> Technically LTP is supposed to support kernel 4.4 and newer, looking
> into kernel git the genhd.h content was moved into blkdev.h in 5.18 so
> ideally we should make it work for both. I suppose that we can make use
> of one of the macros that were moved into blkdev.h and do something as:
>
> #include <linux/blkdev.h>
> #ifndef DISK_NAME_LEN
> # include <linux/genhd.h>
> #endif
>
Good tip, thanks, will add in v2. I was unsure what to do regarding this
and my goal was to compile against v6.11.
> > MODULE_AUTHOR("Márton Németh <nm127@freemail.hu>");
> > diff --git a/testcases/kernel/device-drivers/block/block_dev_user/block_dev.c b/testcases/kernel/device-drivers/block/block_dev_user/block_dev.c
> > index 543c36795cc3b2776c59141023e03ff2c58bd36a..f795fb05b9af71f6a3c748936a15ac3a652db5e3 100644
> > --- a/testcases/kernel/device-drivers/block/block_dev_user/block_dev.c
> > +++ b/testcases/kernel/device-drivers/block/block_dev_user/block_dev.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > /*
> > * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved.
> > *
> > @@ -26,74 +27,59 @@
> > #include <unistd.h>
> > #include <string.h>
> >
> > -#include "test.h"
> > -#include "safe_macros.h"
> > -#include "old_module.h"
> > +#include "tst_test.h"
> > +#include "tst_module.h"
> > +
> > +#define MODULE_NAME "ltp_block_dev"
> > +#define MODULE_NAME_KO MODULE_NAME ".ko"
> >
> > char *TCID = "block_dev";
>
> Isn't this unused as well?
>
Yes, indeed!
> > -int TST_TOTAL = 9;
> >
> > -static const char module_name[] = "ltp_block_dev.ko";
> > static const char dev_result[] = "/sys/devices/ltp_block_dev/result";
> > static const char dev_tcase[] = "/sys/devices/ltp_block_dev/tcase";
> > -static int module_loaded;
> >
> > -static int run_all_testcases;
> > -static const option_t options[] = {
> > - {"a", &run_all_testcases, NULL},
> > +static int module_loaded;
> > +static char* run_all_testcases;
>
> The whitespaces around the asterisk here are wrong. Have fixed all the
> errors and warnings reported by 'make check'?
>
I was not aware of the `check` target, will fix in v2.
> > +static struct tst_option options[] = {
> > + {"a", &run_all_testcases, "-a\tRun all test-cases (can crash the kernel)"},
> > {NULL, NULL, NULL}
> > };
> >
> > static void cleanup(void)
> > {
> > if (module_loaded)
> > - tst_module_unload(NULL, module_name);
> > -}
> > -
> > -static void help(void)
> > -{
> > - printf(" -a Run all test-cases (can crash the kernel)\n");
> > -}
> > -
> > -void setup(int argc, char *argv[])
> > -{
> > - tst_parse_opts(argc, argv, options, help);
> > -
> > - tst_require_root();
> > -
> > - tst_sig(FORK, DEF_HANDLER, cleanup);
> > + tst_module_unload(MODULE_NAME_KO);
> > }
> >
> > -static void test_run(void)
> > +static void run(unsigned int n)
> > {
> > - int off = 0;
> > /*
> > * test-cases #8 and #9 can crash the kernel.
> > * We have to wait for kernel fix where register_blkdev() &
> > * unregister_blkdev() checks the input device name parameter
> > * against NULL pointer.
> > */
>
> Is this still true?
>
Yes. Hence why I kept the same logic and why added "n++".
> > - if (!run_all_testcases)
> > - off = 2;
> > -
> > - tst_module_load(cleanup, module_name, NULL);
> > - module_loaded = 1;
> > -
> > - int i, pass = 0;
> > - for (i = 0; i < TST_TOTAL - off; ++i) {
> > - SAFE_FILE_PRINTF(cleanup, dev_tcase, "%d", i + 1);
> > - SAFE_FILE_SCANF(cleanup, dev_result, "%d", &pass);
> > - tst_resm((pass) ? TPASS : TFAIL, "Test-case '%d'", i + 1);
> > + n++;
> > + if (!run_all_testcases && (n == 8 || n == 9)) {
> > + tst_res(TCONF, "Skipped n = %d", n);
> > + return;
> > }
> > -}
> > -
> > -int main(int argc, char *argv[])
> > -{
> > - setup(argc, argv);
> > -
> > - test_run();
> >
> > - cleanup();
> > + if (!module_loaded) {
> > + tst_module_load(MODULE_NAME_KO, NULL);
> > + module_loaded = 1;
> > + }
> >
> > - tst_exit();
> > + int pass = 0;
> > + SAFE_FILE_PRINTF(dev_tcase, "%d", n);
> > + SAFE_FILE_SCANF(dev_result, "%d", &pass);
> > + tst_res((pass) ? TPASS : TFAIL, "Test-case '%d'", n);
>
> It would be way better if we printed a menaningful information about the
> test here as well, but that would probably require having a third file
> exported by the test module and should ideally be done in an subsequent
> patch.
>
> > }
As per your suggestion, I'm sending v2 without that and then we can
discuss how that can be made better. I chose to do this conversion so as
to learn about LTP and the overall process and it seemed very straight
forward. :)
Thanks for reviewing,
- Ricardo.
> > +
> > +static struct tst_test test = {
> > + .needs_root = 1,
> > + .cleanup = cleanup,
> > + .test = run,
> > + .tcnt = 9,
> > + .options = options,
> > +};
> >
> > ---
> > base-commit: 47aff4decc81ac837fd745278def6883fc2f197b
> > change-id: 20241018-block_dev-c322152e03ad
> >
> > Best regards,
> > --
> > Ricardo B. Marliere <rbm@suse.com>
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
>
> --
> Cyril Hrubis
> chrubis@suse.cz
More information about the ltp
mailing list