Message-ID: <CAKB00G2pEJAFzFBMbCGW4gBNB4UqBpEw22a2Cs4oOdSXUWfsuQ@mail.gmail.com>
Date: Wed, 30 Apr 2025 19:37:42 -0700
From: Bobby Eshleman <bobbyeshleman@...il.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>, Shuah Khan <shuah@...nel.org>, kvm@...r.kernel.org, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	linux-kernel@...r.kernel.org, virtualization@...ts.linux.dev, 
	netdev@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v3] selftests/vsock: add initial vmtest.sh for vsock

On Wed, Apr 30, 2025 at 6:06 AM Stefano Garzarella <sgarzare@...hat.com> wrote:
>
> On Mon, Apr 28, 2025 at 04:48:11PM -0700, Bobby Eshleman wrote:
> >This commit introduces a new vmtest.sh runner for vsock.
> >
> >It uses virtme-ng/qemu to run tests in a VM. The tests validate G2H,
> >H2G, and loopback. The testing tools from tools/testing/vsock/ are
> >reused. Currently, only vsock_test is used.
> >
> >VMCI and hyperv support is automatically built, though not used.
> >
> >Only tested on x86.
> >
> >To run:
> >
> >  $ tools/testing/selftests/vsock/vmtest.sh
>
> I tried and it's working, but I have a lot of these messages in the
> output:
>      dmesg: read kernel buffer failed: Operation not permitted
>
> I'm on Fedora 41:
>
> $ uname -r
> 6.14.4-200.fc41.x86_64
>
> >
> >or
> >
> >  $ make -C tools/testing/selftests TARGETS=vsock run_tests
> >
> >Results:
> >       # linux/tools/testing/selftests/vsock/vmtest.log
> >       setup:  Building kernel and tests
> >       setup:  Booting up VM
> >       setup:  VM booted up
> >       test:vm_server_host_client:guest:       Control socket listening on 0.0.0.0:51000
> >       test:vm_server_host_client:guest:       Control socket connection accepted...
> >       [...]
> >       test:vm_loopback:guest: 30 - SOCK_STREAM retry failed connect()...ok
> >       test:vm_loopback:guest: 31 - SOCK_STREAM SO_LINGER null-ptr-deref...ok
> >       test:vm_loopback:guest: 31 - SOCK_STREAM SO_LINGER null-ptr-deref...ok
> >
> >Future work can include vsock_diag_test.
> >
> >vmtest.sh is loosely based off of tools/testing/selftests/net/pmtu.sh,
> >which was picked out of the bag of tests I knew to work with NIPA.
> >
> >Because vsock requires a VM to test anything other than loopback, this
> >patch adds vmtest.sh as a kselftest itself. This is different than other
> >systems that have a "vmtest.sh", where it is used as a utility script to
> >spin up a VM to run the selftests as a guest (but isn't hooked into
> >kselftest). This aspect is worth review, as I'm not aware of all of the
> >enviroments where this would run.
>
> Yeah, an opinion from net maintainers on this would be great.
>
> >
> >Signed-off-by: Bobby Eshleman <bobbyeshleman@...il.com>
> >---
> >Changes in v3:
> >- use common conditional syntax for checking variables
> >- use return value instead of global rc
> >- fix typo TEST_HOST_LISTENER_PORT -> TEST_HOST_PORT_LISTENER
> >- use SIGTERM instead of SIGKILL on cleanup
> >- use peer-cid=1 for loopback
> >- change sleep delay times into globals
> >- fix test_vm_loopback logging
> >- add test selection in arguments
> >- make QEMU an argument
> >- check that vng binary is on path
> >- use QEMU variable
> >- change <tab><backslash> to <space><backslash>
> >- fix hardcoded file paths
> >- add comment in commit msg about script that vmtest.sh was based off of
> >- Add tools/testing/selftest/vsock/Makefile for kselftest
> >- Link to v2: https://lore.kernel.org/r/20250417-vsock-vmtest-v2-1-3901a27331e8@gmail.com
> >
> >Changes in v2:
> >- add kernel oops and warnings checker
> >- change testname variable to use FUNCNAME
> >- fix spacing in test_vm_server_host_client
> >- add -s skip build option to vmtest.sh
> >- add test_vm_loopback
> >- pass port to vm_wait_for_listener
> >- fix indentation in vmtest.sh
> >- add vmci and hyperv to config
> >- changed whitespace from tabs to spaces in help string
> >- Link to v1: https://lore.kernel.org/r/20250410-vsock-vmtest-v1-1-f35a81dab98c@gmail.com
> >---
> > MAINTAINERS                                |   1 +
> > tools/testing/selftests/vsock/.gitignore   |   1 +
> > tools/testing/selftests/vsock/Makefile     |   9 +
> > tools/testing/selftests/vsock/config.vsock |  10 +
> > tools/testing/selftests/vsock/settings     |   1 +
> > tools/testing/selftests/vsock/vmtest.sh    | 354 +++++++++++++++++++++++++++++
> > 6 files changed, 376 insertions(+)
>
> While applying git warned about trailing spaces, but also checkpatch is
> warning about them, please can you fix at least the errors (and maybe
> the misspelling):
>
> $ ./scripts/checkpatch.pl --strict -g master..HEAD --codespell
> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit
> description?)
> #31:
>         test:vm_server_host_client:guest:       Control socket listening on 0.0.0.0:51000
>
> WARNING: 'enviroments' may be misspelled - perhaps 'environments'?
> #48:
> enviroments where this would run.
> ^^^^^^^^^^^
>
> ERROR: trailing whitespace
> #174: FILE: tools/testing/selftests/vsock/vmtest.sh:47:
> +^Ivm_server_host_client^IRun vsock_test in server mode on the VM and in client mode on the host.^I$
>
> WARNING: line length of 104 exceeds 100 columns
> #174: FILE: tools/testing/selftests/vsock/vmtest.sh:47:
> +       vm_server_host_client   Run vsock_test in server mode on the VM and in client mode on the host.
>
> ERROR: trailing whitespace
> #175: FILE: tools/testing/selftests/vsock/vmtest.sh:48:
> +^Ivm_client_host_server^IRun vsock_test in client mode on the VM and in server mode on the host.^I$
>
> WARNING: line length of 104 exceeds 100 columns
> #175: FILE: tools/testing/selftests/vsock/vmtest.sh:48:
> +       vm_client_host_server   Run vsock_test in client mode on the VM and in server mode on the host.
>
> ERROR: trailing whitespace
> #176: FILE: tools/testing/selftests/vsock/vmtest.sh:49:
> +^Ivm_loopback^I^IRun vsock_test using the loopback transport in the VM.^I$
>
> ERROR: trailing whitespace
> #443: FILE: tools/testing/selftests/vsock/vmtest.sh:316:
> +IFS="^I$
>
> total: 4 errors, 4 warnings, 0 checks, 382 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
>        mechanically convert to the typical style using --fix or --fix-inplace.
>
> NOTE: Whitespace errors detected.
>        You may wish to use scripts/cleanpatch or scripts/cleanfile
>
> Commit ffa17d673263 ("selftests/vsock: add initial vmtest.sh for vsock") has style problems, please review.
>
> NOTE: If any of the errors are false positives, please report
>        them to the maintainer, see CHECKPATCH in MAINTAINERS.
>

This might not have been the right choice, but I adopted the technique
from the pmtu selftest of
using a tab at the end of the help text lines and then splitting with
"IFS=\t\n". I guess the win is that
running the script has the side effect of checking that the help text
has been updated, and the tab
is not visible?

I think we could do something similar with bash arrays though and have
no checkpatch complaints, and
not having to do "git commit --no-verify" to bypass the checks.

> >
> >diff --git a/MAINTAINERS b/MAINTAINERS
> >index 657a67f9031ef7798c19ac63e6383d4cb18a9e1f..3fbdd7bbfce7196a3cc7db70203317c6bd0e51fd 100644
> >--- a/MAINTAINERS
> >+++ b/MAINTAINERS
> >@@ -25751,6 +25751,7 @@ F:     include/uapi/linux/vm_sockets.h
> > F:    include/uapi/linux/vm_sockets_diag.h
> > F:    include/uapi/linux/vsockmon.h
> > F:    net/vmw_vsock/
> >+F:    tools/testing/selftests/vsock/
> > F:    tools/testing/vsock/
> >
> > VMALLOC
> >diff --git a/tools/testing/selftests/vsock/.gitignore b/tools/testing/selftests/vsock/.gitignore
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..1950aa8ac68c0831c12c1aaa429da45bbe41e60f
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/.gitignore
> >@@ -0,0 +1 @@
> >+vsock_selftests.log
> >diff --git a/tools/testing/selftests/vsock/Makefile b/tools/testing/selftests/vsock/Makefile
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..6fded8c4d593541a6f7462147bffcb719def378f
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/Makefile
> >@@ -0,0 +1,9 @@
> >+# SPDX-License-Identifier: GPL-2.0
> >+.PHONY: all
> >+all:
> >+
> >+TEST_PROGS := vmtest.sh
> >+EXTRA_CLEAN := vmtest.log
> >+
> >+include ../lib.mk
> >+
> >diff --git a/tools/testing/selftests/vsock/config.vsock b/tools/testing/selftests/vsock/config.vsock
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..9e0fb2270e6a2fc0beb5f0d9f0bc37158d0a9d23
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/config.vsock
> >@@ -0,0 +1,10 @@
> >+CONFIG_VSOCKETS=y
> >+CONFIG_VSOCKETS_DIAG=y
> >+CONFIG_VSOCKETS_LOOPBACK=y
> >+CONFIG_VMWARE_VMCI_VSOCKETS=y
> >+CONFIG_VIRTIO_VSOCKETS=y
> >+CONFIG_VIRTIO_VSOCKETS_COMMON=y
> >+CONFIG_HYPERV_VSOCKETS=y
> >+CONFIG_VMWARE_VMCI=y
> >+CONFIG_VHOST_VSOCK=y
> >+CONFIG_HYPERV=y
> >diff --git a/tools/testing/selftests/vsock/settings b/tools/testing/selftests/vsock/settings
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..e7b9417537fbc4626153b72e8f295ab4594c844b
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/settings
> >@@ -0,0 +1 @@
> >+timeout=0
> >diff --git a/tools/testing/selftests/vsock/vmtest.sh b/tools/testing/selftests/vsock/vmtest.sh
> >new file mode 100755
> >index 0000000000000000000000000000000000000000..d70b9446e531d6d20beb24ddeda2cf0a9f7e9a39
> >--- /dev/null
> >+++ b/tools/testing/selftests/vsock/vmtest.sh
> >@@ -0,0 +1,354 @@
> >+#!/bin/bash
> >+# SPDX-License-Identifier: GPL-2.0
> >+#
> >+# Copyright (c) 2025 Meta Platforms, Inc. and affiliates
> >+#
> >+# Dependencies:
> >+#             * virtme-ng
> >+#             * busybox-static (used by virtme-ng)
> >+#             * qemu  (used by virtme-ng)
> >+
> >+SCRIPT_DIR="$(cd -P -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd -P)"
> >+KERNEL_CHECKOUT=$(realpath ${SCRIPT_DIR}/../../../..)
> >+QEMU=$(command -v qemu-system-$(uname -m))
> >+VERBOSE=0
> >+SKIP_BUILD=0
> >+VSOCK_TEST=${KERNEL_CHECKOUT}/tools/testing/vsock/vsock_test
> >+
> >+TEST_GUEST_PORT=51000
> >+TEST_HOST_PORT=50000
> >+TEST_HOST_PORT_LISTENER=50001
> >+SSH_GUEST_PORT=22
> >+SSH_HOST_PORT=2222
> >+VSOCK_CID=1234
> >+WAIT_PERIOD=3
> >+WAIT_PERIOD_MAX=20
> >+
> >+QEMU_PIDFILE=/tmp/qemu.pid
> >+
> >+# virtme-ng offers a netdev for ssh when using "--ssh", but we also need a
> >+# control port forwarded for vsock_test.  Because virtme-ng doesn't support
> >+# adding an additional port to forward to the device created from "--ssh" and
> >+# virtme-init mistakenly sets identical IPs to the ssh device and additional
> >+# devices, we instead opt out of using --ssh, add the device manually, and also
> >+# add the kernel cmdline options that virtme-init uses to setup the interface.
> >+QEMU_OPTS=""
> >+QEMU_OPTS="${QEMU_OPTS} -netdev user,id=n0,hostfwd=tcp::${TEST_HOST_PORT}-:${TEST_GUEST_PORT}"
> >+QEMU_OPTS="${QEMU_OPTS},hostfwd=tcp::${SSH_HOST_PORT}-:${SSH_GUEST_PORT}"
> >+QEMU_OPTS="${QEMU_OPTS} -device virtio-net-pci,netdev=n0"
> >+QEMU_OPTS="${QEMU_OPTS} -device vhost-vsock-pci,guest-cid=${VSOCK_CID}"
> >+QEMU_OPTS="${QEMU_OPTS} --pidfile ${QEMU_PIDFILE}"
>
> What about:
>
> QEMU_OPTS=" \
>      -netdev user,id=n0,hostfwd=tcp::${TEST_HOST_PORT}-:${TEST_GUEST_PORT},hostfwd=tcp::${SSH_HOST_PORT}-:${SSH_GUEST_PORT} \
>      -device virtio-net-pci,netdev=n0 \
>      -device vhost-vsock-pci,guest-cid=${VSOCK_CID} \
>      --pidfile ${QEMU_PIDFILE}
> "
>
> Not a strong opinion, just I see too much "QEMU_OPTS="${QEMU_OPTS}" xD
> You can leave it in that way if you prefer.
>

That's totally fine by me, no strong opinion on my side. I tend to do
the string-building thing
across the board because for some reason multiline strings look odd to
me when inside
function scope, and string-building looks like the same pattern
everywhere. Definitely not
a preference I hold tightly though.

> >+KERNEL_CMDLINE="virtme.dhcp net.ifnames=0 biosdevname=0 virtme.ssh virtme_ssh_user=$USER"
> >+
> >+LOG=${SCRIPT_DIR}/vmtest.log
> >+
> >+#             Name                            Description
> >+avail_tests="
> >+      vm_server_host_client   Run vsock_test in server mode on the VM and in client mode on the host.
> >+      vm_client_host_server   Run vsock_test in client mode on the VM and in server mode on the host.
> >+      vm_loopback             Run vsock_test using the loopback transport in the VM.
> >+"
> >+
> >+usage() {
> >+      echo
> >+      echo "$0 [OPTIONS] [TEST]..."
> >+      echo "If no TEST argument is given, all tests will be run."
> >+      echo
> >+      echo "Options"
> >+      echo "  -v: verbose output"
> >+      echo "  -s: skip build"
> >+      echo
> >+      echo "Available tests${avail_tests}"
> >+      exit 1
> >+}
> >+
> >+die() {
> >+      echo "$*" >&2
> >+      exit 1
> >+}
> >+
> >+vm_ssh() {
> >+      ssh -q -o UserKnownHostsFile=/dev/null -p 2222 localhost $*
>
> ${SSH_HOST_PORT} instead of 2222, right?
>

Oops. Yep, will fix.

> The rest LGTM!
>
> Thanks,
> Stefano
>

Thanks for the review!

Best,
Bobby

Powered by blists - more mailing lists