[LTP] Possible fix for LTP Issue #1219
Martin Doucha
mdoucha@suse.cz
Tue Jan 28 14:40:11 CET 2025
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
More information about the ltp
mailing list