[LTP] Possible fix for LTP Issue #1219

[LTP] Possible fix for LTP Issue #1219

[Minear, Coey via ltp]

I created an issue against LTP: https://github.com/linux-test-project/ltp/issues/1219. ‘pevik’ suggested that I send a patch here. I’ll admit that I’m uncertain what form would be preferred, but here’s what I’ll share:
[[PATCH]]
diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c
index 16b3137c0..649bf429a 100644
--- a/testcases/kernel/kvm/kvm_pagefault01.c
+++ b/testcases/kernel/kvm/kvm_pagefault01.c
@@ -214,6 +214,10 @@ static struct tst_test test = {
        .setup = setup,
        .cleanup = tst_kvm_cleanup,
        .needs_root = 1,
+       .needs_drivers = (const char *const []) {
+               "kvm",
+               NULL
+       },
        .supported_archs = (const char *const []) {
                "x86_64",
                NULL
diff --git a/testcases/kernel/kvm/kvm_svm01.c b/testcases/kernel/kvm/kvm_svm01.c
index 32d15526b..f81602567 100644
--- a/testcases/kernel/kvm/kvm_svm01.c
+++ b/testcases/kernel/kvm/kvm_svm01.c
@@ -108,6 +108,10 @@ static struct tst_test test = {
        .test_all = tst_kvm_run,
        .setup = tst_kvm_setup,
        .cleanup = tst_kvm_cleanup,
+       .needs_drivers = (const char *const []) {
+               "kvm",
+               NULL
+       },
        .supported_archs = (const char *const []) {
                "x86_64",
                "x86",
diff --git a/testcases/kernel/kvm/kvm_svm02.c b/testcases/kernel/kvm/kvm_svm02.c
index 6914fdcba..701f2731d 100644
--- a/testcases/kernel/kvm/kvm_svm02.c
+++ b/testcases/kernel/kvm/kvm_svm02.c
@@ -129,6 +129,10 @@ static struct tst_test test = {
        .test_all = tst_kvm_run,
        .setup = tst_kvm_setup,
        .cleanup = tst_kvm_cleanup,
+       .needs_drivers = (const char *const []) {
+               "kvm",
+               NULL
+       },
        .supported_archs = (const char *const []) {
                "x86_64",
                "x86",
diff --git a/testcases/kernel/kvm/kvm_svm03.c b/testcases/kernel/kvm/kvm_svm03.c
index 87164d013..87f9887d8 100644
--- a/testcases/kernel/kvm/kvm_svm03.c
+++ b/testcases/kernel/kvm/kvm_svm03.c
@@ -88,6 +88,9 @@ static void *vm_thread(void *arg)

static void setup(void)
{
+       /* Run the common 'tst_kvm_setup()' first. */
+       tst_kvm_setup();
+
        struct sigaction sa = { .sa_handler = sighandler };
        pthread_mutexattr_t attr;

@@ -155,6 +158,10 @@ static struct tst_test test = {
        .setup = setup,
        .cleanup = cleanup,
        .min_cpus = 2,
+       .needs_drivers = (const char *const []) {
+               "kvm",
+               NULL
+       },
        .supported_archs = (const char *const []) {
                "x86_64",
                "x86",
diff --git a/testcases/kernel/kvm/kvm_svm04.c b/testcases/kernel/kvm/kvm_svm04.c
index e69f0d4be..d8d3bdd96 100644
--- a/testcases/kernel/kvm/kvm_svm04.c
+++ b/testcases/kernel/kvm/kvm_svm04.c
@@ -297,6 +297,10 @@ static struct tst_test test = {
        .test_all = tst_kvm_run,
        .setup = tst_kvm_setup,
        .cleanup = tst_kvm_cleanup,
+       .needs_drivers = (const char *const []) {
+               "kvm",
+               NULL
+       },
        .supported_archs = (const char *const []) {
                "x86_64",
                "x86",
diff --git a/testcases/kernel/kvm/lib_host.c b/testcases/kernel/kvm/lib_host.c
index 8e3d6094e..17215c23b 100644
--- a/testcases/kernel/kvm/lib_host.c
+++ b/testcases/kernel/kvm/lib_host.c
@@ -323,7 +323,14 @@ void tst_kvm_clear_guest_signal(struct tst_kvm_instance *inst)

void tst_kvm_setup(void)
{
-
+       /* Do a quick check that the 'kvm' module is actually loaded by
+          checking for '/dev/kvm'. If that device file is not present, then
+          the module is likely not loaded in which case we should just CONF
+          out.
+       */
+       if (access("/dev/kvm", F_OK) != 0) {
+                tst_brk(TCONF, "The test requires 'kvm' device, which is not loaded.");
+       }
}

void tst_kvm_run(void)
[[/PATCH]]

I’ll admit that this possibly contains parts that you may not want, but it includes the parts of the issue that I raised.

Coey Minear


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

Re: [LTP] Possible fix for LTP Issue #1219

[Petr Vorel]

Hi Coey,

> I created an issue against LTP: https://github.com/linux-test-project/ltp/issues/1219. ‘pevik’ suggested that I send a patch here. I’ll admit that I’m uncertain what form would be preferred, but here’s what I’ll share:
> [[PATCH]]

FYI commit message like this is not much helpful (usually the reason for the
change is important to describe, you don't put your Signed-off-by:), but we can
fix it before merge.

> diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c
> index 16b3137c0..649bf429a 100644
> --- a/testcases/kernel/kvm/kvm_pagefault01.c
> +++ b/testcases/kernel/kvm/kvm_pagefault01.c
> @@ -214,6 +214,10 @@ static struct tst_test test = {
>         .setup = setup,
>         .cleanup = tst_kvm_cleanup,
>         .needs_root = 1,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>         .supported_archs = (const char *const []) {
>                 "x86_64",
>                 NULL
> diff --git a/testcases/kernel/kvm/kvm_svm01.c b/testcases/kernel/kvm/kvm_svm01.c
> index 32d15526b..f81602567 100644
> --- a/testcases/kernel/kvm/kvm_svm01.c
> +++ b/testcases/kernel/kvm/kvm_svm01.c
> @@ -108,6 +108,10 @@ static struct tst_test test = {
>         .test_all = tst_kvm_run,
>         .setup = tst_kvm_setup,
>         .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>         .supported_archs = (const char *const []) {
>                 "x86_64",
>                 "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm02.c b/testcases/kernel/kvm/kvm_svm02.c
> index 6914fdcba..701f2731d 100644
> --- a/testcases/kernel/kvm/kvm_svm02.c
> +++ b/testcases/kernel/kvm/kvm_svm02.c
> @@ -129,6 +129,10 @@ static struct tst_test test = {
>         .test_all = tst_kvm_run,
>         .setup = tst_kvm_setup,
>         .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>         .supported_archs = (const char *const []) {
>                 "x86_64",
>                 "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm03.c b/testcases/kernel/kvm/kvm_svm03.c
> index 87164d013..87f9887d8 100644
> --- a/testcases/kernel/kvm/kvm_svm03.c
> +++ b/testcases/kernel/kvm/kvm_svm03.c
> @@ -88,6 +88,9 @@ static void *vm_thread(void *arg)

> static void setup(void)
> {
> +       /* Run the common 'tst_kvm_setup()' first. */
> +       tst_kvm_setup();
IMHO "kvm" in .needs_drivers is enough, but I might be wrong.
@Martin?

> +
>         struct sigaction sa = { .sa_handler = sighandler };
>         pthread_mutexattr_t attr;

> @@ -155,6 +158,10 @@ static struct tst_test test = {
>         .setup = setup,
>         .cleanup = cleanup,
>         .min_cpus = 2,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>         .supported_archs = (const char *const []) {
>                 "x86_64",
>                 "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm04.c b/testcases/kernel/kvm/kvm_svm04.c
> index e69f0d4be..d8d3bdd96 100644
> --- a/testcases/kernel/kvm/kvm_svm04.c
> +++ b/testcases/kernel/kvm/kvm_svm04.c
> @@ -297,6 +297,10 @@ static struct tst_test test = {
>         .test_all = tst_kvm_run,
>         .setup = tst_kvm_setup,
>         .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>         .supported_archs = (const char *const []) {
>                 "x86_64",
>                 "x86",
> diff --git a/testcases/kernel/kvm/lib_host.c b/testcases/kernel/kvm/lib_host.c
> index 8e3d6094e..17215c23b 100644
> --- a/testcases/kernel/kvm/lib_host.c
> +++ b/testcases/kernel/kvm/lib_host.c
> @@ -323,7 +323,14 @@ void tst_kvm_clear_guest_signal(struct tst_kvm_instance *inst)

> void tst_kvm_setup(void)
> {
> -
> +       /* Do a quick check that the 'kvm' module is actually loaded by
> +          checking for '/dev/kvm'. If that device file is not present, then
> +          the module is likely not loaded in which case we should just CONF
> +          out.
> +       */
> +       if (access("/dev/kvm", F_OK) != 0) {
> +                tst_brk(TCONF, "The test requires 'kvm' device, which is not loaded.");
> +       }
Therefore I would remove this.

Kind regards,
Petr

> }

> void tst_kvm_run(void)
> [[/PATCH]]

> I’ll admit that this possibly contains parts that you may not want, but it includes the parts of the issue that I raised.

> Coey Minear

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

Re: [LTP] Possible fix for LTP Issue #1219

[Martin Doucha]

Hello,
thank you for your patch, there are two code issues below.

As for the preferred form, git provides some useful commands for 
creating patches to be sent via e-mail. First, commit your changes to a 
new branch:
git checkout -b [new_branch_name]
git commit -s

The -s parameter will add your Signed-Off-By tag to the commit message. 
Next, export the patch for submission:
git format-patch --to=ltp@lists.linux.it master

This command will write your commits between the current branch and 
master into patch files which you can edit and add extra comments below 
the commit message, between the "---" marker and the list of modified 
files. Those comments will be removed during merge. For resubmitting 
patches with requested changes, add the -v2 (then -v3, -v4 and so on) 
parameter to format-patch and add a short note what you've changed in 
the extra comments.

Finally, send the patch files:
git send-email 0001-your-changes.patch [...]

Note that you need to configure Git to use your e-mail account. See 
documentation for git-config. Please avoid sending your patches using a 
GUI e-mail client because those usually mess up whitespace and replace 
tabs with spaces. For more details, we follow the same procedure as the 
kernel developers:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

On 27. 01. 25 19:11, Minear, Coey via ltp wrote:
> I created an issue against LTP: https://github.com/linux-test-project/ltp/issues/1219. ‘pevik’ suggested that I send a patch here. I’ll admit that I’m uncertain what form would be preferred, but here’s what I’ll share:
> [[PATCH]]
> diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c
> index 16b3137c0..649bf429a 100644
> --- a/testcases/kernel/kvm/kvm_pagefault01.c
> +++ b/testcases/kernel/kvm/kvm_pagefault01.c
> @@ -214,6 +214,10 @@ static struct tst_test test = {
>          .setup = setup,
>          .cleanup = tst_kvm_cleanup,
>          .needs_root = 1,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>          .supported_archs = (const char *const []) {
>                  "x86_64",
>                  NULL
> diff --git a/testcases/kernel/kvm/kvm_svm01.c b/testcases/kernel/kvm/kvm_svm01.c
> index 32d15526b..f81602567 100644
> --- a/testcases/kernel/kvm/kvm_svm01.c
> +++ b/testcases/kernel/kvm/kvm_svm01.c
> @@ -108,6 +108,10 @@ static struct tst_test test = {
>          .test_all = tst_kvm_run,
>          .setup = tst_kvm_setup,
>          .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>          .supported_archs = (const char *const []) {
>                  "x86_64",
>                  "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm02.c b/testcases/kernel/kvm/kvm_svm02.c
> index 6914fdcba..701f2731d 100644
> --- a/testcases/kernel/kvm/kvm_svm02.c
> +++ b/testcases/kernel/kvm/kvm_svm02.c
> @@ -129,6 +129,10 @@ static struct tst_test test = {
>          .test_all = tst_kvm_run,
>          .setup = tst_kvm_setup,
>          .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>          .supported_archs = (const char *const []) {
>                  "x86_64",
>                  "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm03.c b/testcases/kernel/kvm/kvm_svm03.c
> index 87164d013..87f9887d8 100644
> --- a/testcases/kernel/kvm/kvm_svm03.c
> +++ b/testcases/kernel/kvm/kvm_svm03.c
> @@ -88,6 +88,9 @@ static void *vm_thread(void *arg)
> 
> static void setup(void)
> {
> +       /* Run the common 'tst_kvm_setup()' first. */
> +       tst_kvm_setup();
> +

tst_kvm_setup() may only be used in combination with tst_kvm_run() and 
tst_kvm_cleanup(). This test requires more control over the KVM guest 
than those functions allow so tst_kvm_setup() cannot be used here. 
Please check /dev/kvm directly.

>          struct sigaction sa = { .sa_handler = sighandler };
>          pthread_mutexattr_t attr;
> 
> @@ -155,6 +158,10 @@ static struct tst_test test = {
>          .setup = setup,
>          .cleanup = cleanup,
>          .min_cpus = 2,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>          .supported_archs = (const char *const []) {
>                  "x86_64",
>                  "x86",
> diff --git a/testcases/kernel/kvm/kvm_svm04.c b/testcases/kernel/kvm/kvm_svm04.c
> index e69f0d4be..d8d3bdd96 100644
> --- a/testcases/kernel/kvm/kvm_svm04.c
> +++ b/testcases/kernel/kvm/kvm_svm04.c
> @@ -297,6 +297,10 @@ static struct tst_test test = {
>          .test_all = tst_kvm_run,
>          .setup = tst_kvm_setup,
>          .cleanup = tst_kvm_cleanup,
> +       .needs_drivers = (const char *const []) {
> +               "kvm",
> +               NULL
> +       },
>          .supported_archs = (const char *const []) {
>                  "x86_64",
>                  "x86",
> diff --git a/testcases/kernel/kvm/lib_host.c b/testcases/kernel/kvm/lib_host.c
> index 8e3d6094e..17215c23b 100644
> --- a/testcases/kernel/kvm/lib_host.c
> +++ b/testcases/kernel/kvm/lib_host.c
> @@ -323,7 +323,14 @@ void tst_kvm_clear_guest_signal(struct tst_kvm_instance *inst)
> 
> void tst_kvm_setup(void)
> {
> -
> +       /* Do a quick check that the 'kvm' module is actually loaded by
> +          checking for '/dev/kvm'. If that device file is not present, then
> +          the module is likely not loaded in which case we should just CONF
> +          out.
> +       */
> +       if (access("/dev/kvm", F_OK) != 0) {
> +                tst_brk(TCONF, "The test requires 'kvm' device, which is not loaded.");
> +       }

The check here is correct and necessary but using curly braces around 
one-line statements is against our coding style. See the kernel coding 
style guide for details and use scripts/checkpatch.pl to find similar 
issues.
https://www.kernel.org/doc/html/v4.17/process/coding-style.html

The TCONF message could be as simple as: "KVM is not available".

Also, since /dev/kvm will be opened in read/write mode, you should check 
for R_OK|W_OK access. If the file exists but cannot be opened by the 
user, the test should exit with TCONF.

> }
> 
> void tst_kvm_run(void)
> [[/PATCH]]
> 
> I’ll admit that this possibly contains parts that you may not want, but it includes the parts of the issue that I raised.
> 
> Coey Minear


-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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