In commit fca5e11773 ("qrap: Add probe retry on connection reset
from passt for KubeVirt integration") I just used errno to check if
the connection was reset on recv(), but perror() might set it to
EINVAL if e.g. an underlying logging mechanism fails, so we won't
actually catch the connection reset.
And in case recv() returns 0, errno won't be set, but we're still
using it without resetting it first, which leads to unpredictable
results in that case.
Reset errno before probing with connect(), send() and recv(), and
save it for later checks before calling perror().
Fixes: fca5e11773 ("qrap: Add probe retry on connection reset from passt for KubeVirt integration")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Switch the resolv.conf parsing in conf.c to use the new lineread
implementation. This means that it can now handle a resolv.conf file which
contains blank lines.
There are quite a few other fragilities with the resolv.conf parsing, but
that's out of scope for this patch.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Two places in passt need to read files line by line (one parsing
resolv.conf, the other parsing /proc/net/*. They can't use fgets()
because in glibc that can allocate memory. Instead they use an
implementation line_read() in util.c. This has some problems:
* It has two completely separate modes of operation, one buffering
and one not, the relation between these and how they're activated
is subtle and confusing
* At least in non-buffered mode, it will mishandle an empty line,
folding them onto the start of the next non-empty line
* In non-buffered mode it will use lseek() which prevents using this
on non-regular files (we don't need that at present, but it's a
surprising limitation)
* It has a lot of difficult to read pointer mangling
Add a new cleaner implementation of allocation-free line-by-line
reading in lineread.c. This one always buffers, using a state
structure to keep track of what we need. This is larger than I'd
like, but it turns out handling all the edge cases of line-by-line
reading in C is surprisingly hard.
This just adds the code, subsequent patches will change the existing
users of line_read() to the new implementation.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
This depends on a future change in mbuto to accept external profile
files. Add a file defining what we need for tests and demos, dropping
udhcpc and script as they're not needed anymore, and switch to it.
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
...if there are two or more instances of libvirt in a KubeVirt
scenario using a number of instances of passt, the overlap period
with probing instances of qemu becomes much longer. Switch to 50
retries instead of 5.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
One retry after 100ms was enough for static builds, where qrap
takes a while to start, but it's sometimes not enough with
regular builds. Switch that to five retries with 50ms delay.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In order to probe availability of certain features the Makefile test
compiles a handful of tiny snippets, feeding those in from stdin. However
in one case - the one for -fstack-protector - it forgets to redirect the
output to stdout, meaning it creates a stray '-.s' file when make is
invoked (even make clean).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The use of rm commands in the clean and uninstall targets adds an explicit
leading - to ignore errors. However the built-in RM variable in make is
actually "rm -f" which already ignores errors, so the - isn't neccessary.
Also replace ${RM} with $(RM) which is the more conventional form in
Makefiles.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
pasta, pasta.avx2 and pasta.1 are all generated as a link to the
corresponding passt file. We can consolidate the 3 rules for these targets
into a single pattern rule.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
There are several places which explicitly list the various generated
binaries, even though a $(BIN) variable already lists them. There are
several more places that list all the manpage files, introduce a
$(MANPAGES) variable to remove that repetition as well.
Tweak the generation of pasta.1 as a link to passt.1 so it's not just made
as a side effect of the pasta target.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: add passt.1 and qrap.1 to guest files for distro tests]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The passt/pasta Makefile makes fairly heavy use of GNU make's $(wildcard)
function to locate the sources and headers to build. Using wildcards for
the things to compile is usually a bad idea though: if somehow you end up
with a .c or .h file in your tree you didn't expect it can misbuild in an
exceedingly confusing way. In particular this can sometimes happen if
switching between releases / branches where files have been added or
removed without 100% cleaning the tree.
It also makes life a bit complicated if building multiple different
binaries in the same tree: we already have some rather awkward
$(filter-out) constructions to avoid including qrap.c in the passt build.
Replace use of $(wildcard) with the more idiomatic approach of defining
variables listing all the relevant source files then using that throughout.
In the rule for seccomp.h there was also a bare "*.c" which caused make to
always rebuild that target. Fix that as well.
Similarly, seccomp.sh uses a wildcard to locate the sources, which is
unwise for the same reasons. Make it take the sources to examine on the
command line instead, and have the Makefile pass them in from the same
variables.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
...namely, as connections are discarded or accepted. This was quite
useful to debug an issue with libvirtd failing to start qemu (because
passt refused the new connection) as a previous qemu instance was
still active.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
KubeVirt uses libvirt to start qrap in its current draft integration
(https://github.com/kubevirt/kubevirt/pull/7849/), and libvirtd
starts qrap three times every time a new virtual machine is created:
once on domain creation, and twice on domain start (for "probing")
and to finally start it for real.
Very often, a subsequent invocation of qrap happen before the
previously running instance of qemu terminates, which means that
passt will refuse the new connection as the old one is still active.
Introduce a single retry with a 100ms delay to work around this. This
should be checked again once native libvirt support is there, and
that point qrap will have no reason to exist anymore.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
New from Cppcheck 2.8: all the fields of struct msg that are not
directly manipulated are now reported as unused, which is kind of
correct as those fields are used as a blob "copied" from request
to response and not as separate fields.
However, keeping the message composition explicit is probably
desirable, and adding inline suppressions makes the whole thing
rather unreadable, so just suppress unusedStructMember warnings for
dhcp.c, while also adding a suppression for unmatched suppressions
to keep earlier versions of Cppcheck happy.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A number of the testcases use options specific the OpenBSD version of
netcat. That's available in Debian, but not easily available in Fedora.
Switch the pasta tests to using the nmap version of netcat (a.k.a. ncat).
This is easily available in both Debian and Fedora, and appears to be a
bit more modern and maintained as well.
ncat generally requires explicit listen addresses (which is good for
clarity anywhere). Its default options appear to remove the need for the
-N and -q options.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: changed one ncat listening address to IPv6 loopback]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
For some reason, the passt/pasta tests and examples use dhclient for
DHCPv6, but in most cases use udhcpc for DHCPv4. Change it to use dhclient
for both DHCPv4 and DHCPv6. This means one less tool we need for testing,
plus dhclient is easily available on Fedora whereas udhcpc is not.
Note that the passt tests still rely on udhcpc indirectly because mbuto
wants to put it into the guest images it generates.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
A number of tests and examples use dhclient in both IPv4 and IPv6 modes.
We use "dhclient -6" for IPv6, but usually just "dhclient" for IPv4. Add
an explicit "-4" argument to make it more clear and explicit.
In addition, when dhclient is run from within pasta it usually won't be
"real" root, and so will not have access to write the default global pid
file. This results in a mostly harmless but irritating error:
Can't create /var/run/dhclient.pid: Permission denied
We can avoid that by using the --no-pid flag to dhclient.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
ip(8)'s ability to take abbreviated arguments (e.g. "li sh" instead of
"link show") is very handy when using it interactively, but it doesn't make
for very readable scripts and examples when shown that way.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
distro/fedora contains two versions of the basic tests, used for different
Fedora versions. One uses explicit listening address for netcat in some
extra places, the other does not. Apparently the older netcat versions
didn't require the explicit addresses. Not supplying addresses doesn't
test anything useful though, just a detail in netcat's behaviour. So,
it's cleaner to just always supply explicit addresses.
In addition, we're explicitly expecting the nmap version of ncat, also
known as "ncat". So, it's more explicit what we're after if we invoke it
via that name rather than "nc", which will go via an /etc/alternatives
link.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Fix port argument in distro_quick_pasta_test{,_fedora34} too]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If the first packet_get() call doesn't assign len, the second one
will also return NULL, but gcc doesn't see this.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In conf_runas(), Coverity reports that we might dereference uid and
gid despite possibly being NULL (CWE-476) because of the check after
the first sscanf(). They can't be NULL, but I actually wanted to
check that UID and GID are non-zero (the user could otherwise pass
--runas root:root and defy the whole mechanism).
Later on, we have the same type of warning for 'gr': it's compared
against NULL, so it might be NULL, which is actually the case: but
in that case, we don't dereference it, because we'll return -ENOENT
right away. Rewrite the clause to silence the warning.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
gcc 12.1.x (e.g. current OpenSUSE Tumbleweed, x86_64 only,
gcc-12-1.4.x86_64) reports:
tcp.c: In function ‘tcp_send_flag’:
tcp.c:1014:9: warning: writing 16 bytes into a region of size 0 [-Wstringop-overflow=]
1014 | memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6));
| ^
tcp.c:559:24: note: at offset -16 into destination object ‘low_rtt_dst’ of size 128
559 | static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE];
|
but 'hole' can't be -1, because the low_rtt_dst table is guaranteed
to have a hole: if we happened to write to the last entry, we'll go
back to index 0 and clear that one.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
On some systems, user and group "nobody" might not be available. The
new --runas option allows to override the default "nobody" choice if
started as root.
Now that we allow this, drop the initgroups() call that was used to
add any additional groups for the given user, as that might now
grant unnecessarily broad permissions. For instance, several
distributions have a "kvm" group to allow regular user access to
/dev/kvm, and we don't need that in passt or pasta.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
With current OpenSUSE Tumbleweed on aarch64 (gcc-12-1.3.aarch64) and
on x86_64 (gcc-12-1.4.x86_64), but curiously not on armv7hl
(gcc-12-1.3.armv7hl), gcc warns about using the _pointer_ to the
802.3 header to write the whole frame to the tap descriptor:
reading between 62 and 4294967357 bytes from a region of size 14
which is bogus:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103483
Probably declaring udp_sock_fill_data_v{4,6}() as noinline would
"fix" this, but that's on the data path, so I'd rather not. Use
a gcc pragma instead.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Having all those 'echo $?' is rather distracting in demos.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Sefano Brivio <sbrivio@redhat.com>
...there are no 'test' directives in demo, and this causes a
script failure.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
'sleep' always needs an argument, this was meant to introduce
a 2 seconds delay.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
There's no need to return non-zero if there have been failures in
run(), because the exit value is already determined from the number
of failures reported in the log file.
Return zero, so that this doesn't cause the script to fail, given we
now run it with -e.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
There are a few occurrences of this assignment, which are needed to
re-add ::1 as loopback address after the MTU has been increased
back from a value below 1280 bytes.
This one, however, is redundant, and causes an error in the
execution.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A number of individual test cases use '*out' commands to check for success
of specific commands they've issued. Now that the test harness is testing
for success of all issued commands as a matter of course, we no longer need
to do this.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Now that we have pane_status to check the success of commands issued to
panes, we can more easily check for the success of the 'which' commands
used to check tool availability, rather than constructing, then parsing
special "skip" output.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
When we use pane_wait to wait for a command issued to a tmux pane to finish
we have no idea whether the command succeeded or not. This means that the
test scripts can keep running long after the point something vital has
failed, making it difficult to work out what went wrong.
Add a new pane_status command that checks for success of the issued command
and use it in most places instead of pane_wait. We still need explicit
pane_wait where we're gathering explicit output with pane_parse, because
the way we check the status with 'echo $?' means we lose track of that
output.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio:
- instead of quitting the script, make a test fail if a command
issued in a pane fails during a test, and loop until the status code is
numeric in pane_status() as a hack to make it a bit more robust
- retain usage of pane_wait() in iperf3 and teardown functions as we
interrupt iperf3, passt, and pasta, so a non-zero exit code is expected
- drop bogus ns_{1,2}_wait() calls in teardown_two_guests(), those
functions were never implemented
- use pane_status() for "guest" test directives too
]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Most commands issued during the testing scripts aren't explicitly checked
for errors. Therefore, if they fail, the shell will just keep on
executing. This makes it difficult to figure out where things started
going wrong if things fall over.
Run the whole script with the set -e mode so that it will exit in the case
of any (unchecked) failing command. To make this work we do need to add
explicit checks / fallbacks for some commands which we expect to fail.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: use sh -e instead of setting -e later, so that we don't miss
anything before set -e is issued]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
pane_parse() attempts to grab the output from the last command issued
into a tmux pane. It strips out control characters using tr, which in
particular includes the final \r\n. However, this won't fully strip
out terminal escape sequences. In particular this breaks if the shell
in the pane is bash, with enable-bracketed-paste enabled in readline.
That issues terminal sequences to enable and disable bracketed paste
mode around every shell prompt.
We can work around this because these escapes are followed by a \r
(CR). More generally, it seems reasonable to assume that any terminal
shenanigans followed by a CR, but not an LF is supposed to be hidden.
So, use sed to strip everything before the second last CR. We still
need the tr to remove the final \r\n from the string (sed processes a
line at a time, and doesn't consider the CRLF part of the buffer it's
processing).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: modify regexp to keep foo\r\r\n unchanged, by matching on at
least one CR and a non-CR afterwards: that's the usual output pattern
for bash on Debian 8 and Debian 9]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
run_term() uses tmux set-option -g to globally set the default shell.
Unfortunately this hits a chicken-and-egg problem that's common with many
of tmux's session options. If there isn't already a tmux server running,
we can't connect to set the option. If we attempt this after starting our
session (and therefore the server), then the session will already be
started with the previous default shell.
In any case it's not a good idea to set tmux global options, since that
might interfere with whatever else the user is doing in tmux. So, instead
set the default-shell option locally to the session after starting it. To
make sure we get the right shell for our initial script, explicitly invoke
/bin/sh to interpret it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The semantics of tmux's update-environment option are a bit confusing.
It says it means the given variables are copied into the session
environment from the source environment, but it's not entirely clear
what the "source" environment means.
From my experimentation it appeast to be the environment from which
the tmux *server* is launched, not the one issuing the 'new-session'
command. That makes it pretty much useles, certainly in our case where
we have no way of knowing if the user has pre-existing tmux sessions.
Instead use the new-session -e option to explicitly pass in the variables
we want to propagate.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The DEBUG option for tests/run enables debugging options to passt/pasta,
however that doesn't help with debugging the test scripts themselves, which
are fairly fragile.
Extend the DEBUG option so it also prints information on each command in
the test scripts to make it easier to work out where things are falling
over.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The XVFB variable is initialized at the beginning of test/run then never
used again. I'm assuming it's a leftover from some ealier iteration.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Ignore various files generated during build or test.
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reflect the recent changes in the Podman adaptation (no port
forwarding by default).
It turns out that by running two iperf3 processes, sometimes
slirp4netns blocks the second connection until the first test is
done, thus doubling the throughput. Use a single process for
slirp4netns with slirp4netns port handling.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Rebase the patch for Podman on top of current upstream, and:
- add support for configuration of specific addresses for forwarded
ports
- by default, disable port forwarding, and reflect this in the man
page changes
- adjust processing to a new, incompatible format for port storage,
which I couldn't actually track down to a specific commit, but
that resulted in https://github.com/containers/podman/issues/13643
and commit eedaaf33cdbf ("fix slirp4netns port forwarding with
ranges")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
