We define our own ffsl() as a weak symbol, in case our C library doesn't
include it. On glibc systems which *do* include it, this causes a cppcheck
warning because unsurprisingly our version doesn't pick the same argument
names. Convert the suppression for this into an inline suppression.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Some versions of cppcheck could errneously report a NULL pointer deference
inside a sizeof(). This is now fixed in cppcheck upstream[0]. For systems
using an affected version, add a suppression to work around the bug. Also
add an unmatchedSuppression suppression so the suppression itself doesn't
cause a warning if you *do* have a fixed cppcheck.
[0] https://github.com/danmar/cppcheck/pull/4471
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Add the -Wextra -pedantic and -std=c99 flags when compiling the nsholder
test helper to get extra compiler checks, like we already use for the
main source code.
While we're there, fix some %d (signed) printf descriptors being used
for unsigned values (uid_t and gid_t). Pointed out by cppcheck.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
cppcheck points out that qrap's main shadows the global err() function with
a local. Rename it to rc to avoid the clash.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The counter 'i' is used in a number of places in conf_ports(), but in one
of those we unnecessarily shadow it in an inner scope. We could re-use the
same 'i' every time, but each use is logically separate, so instead remove
the outer declaration and declare it locally in each of the clauses where
we need it.
While we're there change it from a signed to unsigned int, since it's used
to iterate over port numbers which are generally treated as unsigned.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Minor style improvement suggested by cppcheck.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
conf_runas() handles several of the different possible cases for the
--runas argument in a slightly odd order. Although it can parse both
numeric UIDs/GIDs and user/group names, it can't parse a numeric UID
combined with a group name or vice versa. That's not obviously useful, but
it's slightly surprising gap to have.
Rework the parsing to be more systematic: first split the option into
user and (optional) group parts, then separately parse each part as either
numeric or a name. As a bonus this removes some clang-tidy warnings.
While we're there also add cppcheck suppressions for getpwnam() and
getgrnam(). It complains about those because they're not reentrant.
passt is single threaded though, and is always likely to be during
this initialization code, even if we multithread later.
There were some existing suppressions for these in the cppcheck
invocation but they're no longer up to date. Replace them with inline
suppressions which, being next to the code, are more likely to stay
correct.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
dhcpv6.c contains a number of structures which represent actual DHCPv6
packets as they appear on the wire, which will break if the structures
don't have exactly the in-memory layout we expect.
Therefore, we should mark these structures as ((packed)). The contents of
them means this is unlikely to change the layout in practice - and since
it was working, presumably didn't on any arch we were testing on. However
it's not impossible for the compiler on some arch to insert unexpected
padding in one of these structures, so we should be explicit.
clang-tidy warned about this since we were using memcmp() to compare some
of these structures, which it thought might not have a unique
representation.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Stop ignoring the return codes from sigaction() and signal(). Unlikely to
happen in practice, but if it ever did it could lead to really hard to
debug problems. So, take clang-tidy's advice and check for errors here.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
clang-tidy isn't quite clever enough to figure out that getenv("SHELL")
will return the same thing both times here, which makes it conclude that
shell could be NULL, causing problems later.
It's a bit ugly that we call getenv() twice in any case, so rework this in
a way that clang-tidy can figure out shell won't be NULL.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
clang-tidy complains that we're not checking the result of vfprintf in
logfn(). There's not really anything we can do if this fails here, so just
suppress the error with a cast to void.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
conf_ports() parses ranges of ports for the -t, -u, -T and -U options.
The code is quite difficult to the follow, to the point that clang-tidy
and cppcheck disagree on whether one of the pointers can be NULL at some
points.
Rework the code with the use of two new helper functions:
* parse_port_range() operates a bit like strtoul(), but can parse a whole
port range specification (e.g. '80' or '1000-1015')
* next_chunk() does the necessary wrapping around strchr() to advance to
just after the next given delimiter, while cleanly handling if there
are no more delimiters
The new version is easier to follow, and also removes some cppcheck
warnings.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Debian and similar distros put target specific header files in
/usr/include/<arch-vendor-os>, rather than directly in /usr/include. Add
this directory to the includes for cppcheck so it can find them.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We do some manipulation of the output of cc -v to get the target triple
for the platform, to locate headers for cppcheck. However, we can get
this more easily with cc -dumpmachine - and in fact we do so elsewhere in
the Makefile.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Adding the --quiet option to cppcheck makes the actual errors and warnings
easier to find.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
make cppcheck takes a long time, because it checks a large number of
different configurations. It's assembling this very large set of
configurations not because of conditionals in the passt code itself,
but from those in the system headers. By adding --config-exclude
directives to stop considering those configs, make cppcheck becomes
around 60x faster on my system.
Similarly, any problems that are found in the system headers are not our
problem, and so we can uniformly suppress them, rather than having specific
suppressions for particular problems in particular files (which might not
be correct for all different distro / version combinations either).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This check complains about any identifier of less than 3 characters. For
locals and parameters this is often pointlessly verbose. Disable it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This loop goes through and gives a numeric label to each pane, even though
we name the panes properly shortly thereafter. Looks like a leftover from
some earlier version. Remove it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Many of our tests are based around performing transfers of sample data
across passt/pasta created links. The data flow here can be a bit
hard to follow since, e.g. we create a file transfer it to the guest,
then transfer it back to the host across several different tests.
This also means that the test cases aren't independent of each other.
Because we don't have the original file available at both ends in some
cases, we compare them by generating md5sums at each end and comparing
them, which is a bit complicated.
Make a number of changes to simplify this:
1. Pre-generate the sample data files as a test asset, rather than
building them on the fly during the tests proper
2. Include the sample data files in the mbuto guest image
3. Because we have good copies of the original data available in all
contexts, we can now simply use 'cmp' to check if the transfer
has worked, avoiding md5sum complications.
4. Similarly we can always use the original copy of the sample data
on the send side of each transfer, meaning that the tests become
more independent of each other.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The setup functions for passt_in_ns and two_guests perform some fairly slow
dhclient calls to configure the network in the namespace before starting
the guest. This isn't really part of the tests, just necessary for the
operations later.
We can simplify and speed this up a bit by using pasta's '--config-net'
option to configure the networking for us. As a bonus this means we have
at least a minimal test of the --config-net option itself.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When we start passt or pasta, it may take a short time to be ready to
handle packets, especially if running under valgrind. We have a
number of semi-arbitrary fixed sleeps to account for this.
We can do this more robustly by exploiting the fact that pasta/passt
doesn't write its pidfile until it's ready to go, so if we wait for
the pidfile to be created, we can proceed with confidence.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
These are hangovers from older ways of shutting down the pasta/passt
processes and no longer serve any purpose.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Add a shell helper function to wait for some command to succeed - typically
a test for something to be done by a background process. Use it in the
context code which waits for the guest to respond to ssh-over-vsock
connections.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
ICMP echo request and reply packets include a 16-bit 'id' value. We have
some arrays indexed by this id value. Unfortunately we size those arrays
with USHRT_MAX (65535) when they need to be sized by the total number of
id values (65536). This could lead to buffer overruns. Resize the arrays
correctly, using a new define for the purpose.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Port numbers (for both TCP and UDP) are 16-bit, and so fit exactly into a
'short'. USHRT_MAX is therefore the maximum port number and this is widely
used in the code. Unfortunately, a lot of those places don't actually
want the maximum port number (USHRT_MAX == 65535), they want the total
number of ports (65536). This leads to a number of potentially nasty
consequences:
* We have buffer overruns on the port_fwd::delta array if we try to use
port 65535
* We have similar potential overruns for the tcp_sock_* arrays
* Interestingly udp_act had the correct size, but we can calculate it in
a more direct manner
* We have a logical overrun of the ports bitmap as well, although it will
just use an unused bit in the last byte so isnt harmful
* Many loops don't consider port 65535 (which does mitigate some but not
all of the buffer overruns above)
* In udp_invert_portmap() we incorrectly compute the reverse port
translation for return packets
Correct all these by using a new NUM_PORTS defined explicitly for this
purpose.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Port numbers are unsigned values, but we're storing them in (signed) int
variables in some places. This isn't actually harmful, because int is
large enough to hold the entire range of ports. However in places we don't
want to use an in_port_t (usually to avoid overflow on the last iteration
of a loop) it makes more conceptual sense to use an unsigned int. This will
also avoid some problems with later cleanups.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
conf_ports() switches on the optname argument to select the target array
for several updates. Now that all these maps are in a common structure, we
can simplify by just passing in a pointer to the whole struct port_fwd to
update.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Now that we've delayed initialization of the UDP specific "reverse" map
until udp_init(), the only difference between the various 'remap' functions
used in conf_ports() is which array they target. So, simplify by open
coding the logic into conf_ports() with a pointer to the correct mapping
array.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Because it's connectionless, when mapping UDP ports we need, in addition
to the table of deltas for destination ports needed by TCP, we need an
inverted table to translate the source ports on return packets.
Currently we fill out the inverted table at the same time we construct the
main table in udp_remap_to_tap() and udp_remap_to_init(). However, we
don't use either table until after we've initialized UDP, so we can delay
the construction of the reverse table to udp_init(). This makes the
configuration more symmetric between TCP and UDP which will enable further
cleanups.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
The configuration for how to forward ports in and out of the guest/ns is
divided between several different variables. For each connect direction
and protocol we have a mode in the udp/tcp context structure, a bitmap
of which ports to forward also in the context structure and an array of
deltas to apply if the outward facing and inward facing port numbers are
different. This last is a separate global variable, rather than being in
the context structure, for no particular reason. UDP also requires an
additional array which has the reverse mapping used for return packets.
Consolidate these into a re-used substructure in the context structure.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
enum conf_port_type is local to conf.c and is used to track the port
forwarding mode during configuration. We don't keep it around in the
context structure, however the 'init_detect_ports' and 'ns_detect_ports'
fields in the context are based solely on this. Rather than changing
encoding, just include the forwarding mode into the context structure.
Move the type definition to a new port_fwd.h, which is kind of trivial at
the moment but will have more stuff later.
While we're there, "conf_port_type" doesn't really convey that this enum is
describing how port forwarding is configured. Rename it to port_fwd_mode.
The variables (now fields) of this type also have mildly confusing names
since it's not immediately obvious whether 'ns' and 'init' refer to the
source or destination of the packets. Use "in" (host to guest / init to
ns) and "out" (guest to host / ns to init) instead.
This has the added bonus that we no longer have locals 'udp_init' and
'tcp_init' which shadow global functions.
In addition, add a typedef 'port_fwd_map' for a bitmap of each port number,
which is used in several places.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Define the target machine architecture in lowercase.
The name of the executable qemu-system-* is defined from the build flags
and should be in lowercase:
( "qemu-system-" ARCH ),
I.e. qemu-system-x86_64 instead of qemu-system-X86_64. Otherwise, the
exec call will fail.
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
As suggested by David: those emojis might not be entirely obvious.
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
It's not used anymore. While at it, fix the function name in the
comment to perf_report_append_js().
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
That's the default port for iperf3, which also means that it's quite
likely in use on my test machine. Use different port numbers: recycle
the scheme we use in tests for passt and pasta's demo, use 5221-5224
(a bit shorter) for the slirp4netns container in Podman's demo.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A cd to __STATEDIR__ results in a rather long command, that's not
very readable. Jump between directories using .. and relative paths,
once we're there.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The test_logs directory contains a directory: fix the wildcard so
that scp doesn't fail.
Terminal capture files are now deleted every time we re-run the
demo script: upload CI's .cast file before it's gone.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We won't necessarily run mbuto as part of regular tests: it can also
be used for demos or out-of-tree tests.
To keep the profile simple, leave the whole sshd setup there, which
is otherwise harmless, but don't fail if guest-key.pub is missing in
the current directory.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If we use dhclient without creating a complete network configuration,
systemd-resolved will stop working after a while, and this sometimes
happens while we're still installing packages.
Disable it, together with systemd-networkd, while taking care of
removing the dhclient hook that prevents overriding /etc/resolv.conf.
While at it, it looks like removing snapd and needrestart actually
takes more time than keeping them: drop that line.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
I'm not sure why, but dhclient hangs otherwise. This reflects what we
do in the passt_in_ns setup steps.
Eventually, this whole block could go away if we let pasta configure
this network namespace with --config-net.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Starting tcp_rr, tcp_crr, udp_rr servers in the guest takes a bit
longer than starting the corresponding clients on the host, and we
end up starting clients before servers unless we add a delay there.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
