Laurent's recent changes mean we use IO vectors much more heavily in the
TCP code. In many of those cases, and few others around the code base,
individual iovs of these vectors are constructed to exactly cover existing
variables or fields. We can make initializing such iovs shorter and
clearer with a macro for the purpose.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
At various points we need to track the lengths of a packet including or
excluding various different sets of headers. We don't always use the same
variable names for doing so. Worse in some places we use the same name
for different things: e.g. tcp_fill_headers[46]() use ip_len for the
length including the IP headers, but then tcp_send_flag() which calls it
uses it to mean the IP payload length only.
To improve clarity, standardise on these names:
dlen: L4 protocol payload length ("data length")
l4len: plen + length of L4 protocol header
l3len: l4len + length of IPv4/IPv6 header
l2len: l3len + length of L2 (ethernet) header
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
csum_ip4_header() takes the packet length as a network endian value. In
general it's very error-prone to pass non-native-endian values as a raw
integer. It's particularly bad here because this differs from other
checksum functions (e.g. proto_ipv4_header_psum()) which take host native
lengths.
It turns out all the callers have easy access to the native endian value,
so switch it to use host order like everything else.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In general, it's much less error-prone to have the endianness of values
implied by the type, rather than just noting it in comments. We can't
always easily avoid it, because C, but we can do so when possible. struct
in_addr and in6_addr are always encoded network endian, so noting it
explicitly isn't useful. Remove them.
In some cases we also have endianness notes on uint8_t parameters, which
doesn't make sense: for a single byte endianness is irrelevant. Remove
those too.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Most times we send frames to the guest it goes via tap_send_frames().
However "slow path" protocols - ARP, ICMP, ICMPv6, DHCP and DHCPv6 - go
via tap_send().
As well as being a semantic duplication, tap_send() contains at least one
serious problem: it doesn't properly handle short sends, which can be fatal
on the qemu socket connection, since frame boundaries will get out of sync.
Rewrite tap_send() to call tap_send_frames(). While we're there, rename it
tap_send_single() for clarity.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We can both remove some variables which differ from others only in type,
and slightly improve type safety.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
tap_send_frames() takes a vector of buffers and requires exactly one frame
per buffer. We have future plans where we want to have multiple buffers
per frame in some circumstances, so extend tap_send_frames() to take the
number of buffers per frame as a parameter.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Improve comment to rembufs calculation]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In tap_send_frames(), if we failed to send all the frames, we must
only log the frames that have been sent, not all the frames we wanted
to send.
Fixes: dda7945ca9 ("pcap: Handle short writes in pcap_frame()")
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Use ethhdr rather than tap_hdr.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Message-ID: <20240303135114.1023026-9-lvivier@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We can find the same function to compute the IPv4 header
checksum in tcp.c, udp.c and tap.c
Use the function defined for tap.c, csum_ip4_header(), but
with the code used in tcp.c and udp.c as it doesn't need a fully
initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Message-ID: <20240303135114.1023026-7-lvivier@redhat.com>
[dwg: Fix weird cppcheck regression; it appears to be a problem
in pre-existing code, but somehow this patch is exposing it]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Introduce ip.[ch] file to encapsulate IP protocol handling functions and
structures. Modify various files to include the new header ip.h when
it's needed.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Message-ID: <20240303135114.1023026-5-lvivier@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The "tap" interface, whether it's actually a tuntap device or a qemu
socket, presents a virtual external link between different network hosts.
Hence, loopback addresses make no sense there. However, nothing prevents
the guest from putting bogus packets with loopback addresses onto the
interface and it's not entirely clear what effect that will have on passt.
Explicitly test for such packets and drop them.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When we determine we have sent a partial frame in tap_send_frames_passt(),
we call tap_send_remainder() to send the remainder of it. The logic in
that function is very similar to that in the more general write_remainder()
except that it uses send() instead of write()/writev(). But we are dealing
specifically with the qemu socket here, which is a connected stream socket.
In that case write()s do the same thing as send() with the options we were
using, so we can just reuse write_remainder().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Currently pcap_frame() assumes that if write() doesn't return an error, it
has written everything we want. That's not necessarily true, because it
could return a short write. That's not likely to happen on a regular file,
but there's not a lot of reason not to be robust here; it's conceivable we
might want to direct the pcap fd at a named pipe or similar.
So, make pcap_frame() handle short frames by using the write_remainder()
helper.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[sbrivio: Formatting fix, and avoid gcc warning in pcap_frame()]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Several of the IOV functions in iov.c, and also tap_send_frames_passt()
needs to determine which buffer element a byte offset into an IO vector
lies in. Split this out into a helper function iov_skip_bytes().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Sufficiently recent cppcheck (I'm using 2.13.0) seems to have added another
warning for pointer variables which could be pointer to const but aren't.
Use this to make a bunch of variables const pointers where they previously
weren't for no particular reason.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Types size_t and ssize_t are not necessarily long, it depends on the
architecture.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
tap_send_frames_pasta() sends frames to the namespace by sending them to
our the /dev/tap device. If that write() returns an error, we already
handle it. However we don't handle the case where the write() returns
short, meaning we haven't successfully transmitted the whole frame.
I don't know if this can ever happen with the kernel tap device, but we
should at least report the case so we don't get a cryptic failure. For
the purposes of the return value for tap_send_frames_pasta() we treat this
case as though it was an error (on the grounds that a partial frame is no
use to the namespace).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Since a469fc39 ("tcp, tap: Don't increase tap-side sequence counter for
dropped frames") we've handled more gracefully the case where we get data
from the socket side, but are temporarily unable to send it all to the tap
side (e.g. due to full buffers).
That code relies on tap_send_frames() returning the number of frames it
successfully sent, which in turn gets it from tap_send_frames_passt() or
tap_send_frames_pasta().
While tap_send_frames_passt() has returned that information since b62ed9ca
("tap: Don't pcap frames that didn't get sent"), tap_send_frames_pasta()
always returns as though it succesfully sent every frame. However there
certainly are cases where it will return early without sending all frames.
Update it report that properly, so that the calling functions can handle it
properly.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
For now, packets passed to the various *_tap_handler() functions always
come from the single "tap" interface. We want to allow the possibility to
broaden that in future. As preparation for that, have the code in tap.c
pass the pif id of the originating interface to each of those handler
functions.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
...so that we'll retry sending them, instead of more-or-less silently
dropping them. This happens quite frequently if our sending buffer on
the UNIX domain socket is heavily constrained (for instance, by the
208 KiB default memory limit).
It might be argued that dropping frames is part of the expected TCP
flow: we don't dequeue those from the socket anyway, so we'll
eventually retransmit them.
But we don't need the receiver to tell us (by the way of duplicate or
missing ACKs) that we couldn't send them: we already know as
sendmsg() reports that. This seems to considerably increase
throughput stability and throughput itself for TCP connections with
default wmem_max values.
Unfortunately, the 16 bits left as padding in the frame descriptors
we use internally aren't enough to uniquely identify for which
connection we should update sequence numbers: create a parallel
array of pointers to sequence numbers and L4 lengths, of
TCP_FRAMES_MEM size, and go through it after calling sendmsg().
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Newer versions of cppcheck (as of 2.12.0, at least) added a warning for
pointers which could be declared to point at const data, but aren't.
Based on that, make many pointers throughout the codebase const.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In both tap4_handler() and tap6_handler(), once we've sorted incoming l3
packets into "sequences", we then step through all the packets in each DUP
sequence calling udp_tap_handler(). Or so it appears.
In fact, udp_tap_handler() doesn't take an index and always starts with
packet 0 of the sequence, even if called repeatedly. It appears to be
written with the idea that the struct pool is a queue, from which it
consumes packets as it processes them, but that's not how the pool data
structure works.
Correct this by adding an index parameter to udp_tap_handler() and altering
the loops in tap.c to step through the pool properly.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In both tap4_handler() and tap6_handler(), once we've sorted incoming l3
packets into "sequences", we then step through all the packets in each TCP
sequence calling tcp_tap_handler(). Or so it appears.
In fact, tcp_tap_handler() doesn't take an index and always looks at packet
0 of the sequence, except when it calls tcp_data_from_tap() to process
data packets. It appears to be written with the idea that the struct pool
is a queue, from which it consumes packets as it processes them, but that's
not how the pool data structure works - they are more like an array of
packets.
We only get away with this, because setup packets for TCP tend to come in
separate batches (because we need to reply in between) and so we only get
a bunch of packets for the same connection together when they're data
packets (tcp_data_from_tap() has its own loop through packets).
Correct this by adding an index parameter to tcp_tap_handler() and altering
the loops in tap.c to step through the pool properly.
Link: https://bugs.passt.top/show_bug.cgi?id=68
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
l3_len was calculated from the ethernet frame size, and it
was assumed to be equal to the length stored in an IP packet.
But if the ethernet frame is padded, then l3_len calculated
that way can only be used as a bound check to validate the
length stored in an IP header. It should not be used for
calculating the l4_len.
This patch makes sure the small padded ethernet frames are
properly processed, by trusting the length stored in an IP
header.
Link: https://bugs.passt.top/show_bug.cgi?id=73
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Because packets sent on the tap interface will always be going to the
guest/namespace, we more-or-less know what address they'll be going to. So
we pre-fill this destination address in our header buffers for IPv4. We
can't do the same for IPv6 because we could need either the global or
link-local address for the guest. In future we're going to want more
flexibility for the destination address, so this pre-filling will get in
the way.
Change the flow so we always fill in the IPv4 destination address for each
packet, rather than prefilling it from proto_update_l2_buf(). In fact for
TCP we already redundantly filled the destination for each packet anyway.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The tap code passes the IPv4 or IPv6 destination address of packets it
receives to the protocol specific code. Currently that protocol code
doesn't use the source address, but we want it to in future. So, in
preparation, pass the IPv4/IPv6 source address of tap packets to those
functions as well.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In tap6_handler() saddr is initialized to the IPv6 source address from the
incoming packet. However part way through, but before organizing the
packet into a "sequence" we set it unconditionally to the guest's assigned
address. We don't do anything equivalent for IPv4.
This doesn't make a lot of sense: if the guest is using a different source
address it makes sense to consider these different sequences of packets and
we shouldn't try to combine them together.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Spotted by Coverity, relatively harmless.
Fixes: e01759e2fa ("tap: Explicitly drop IPv4 fragments, and give a warning")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Currently we have a single epoll event type for the "tap" fd, which could
be either a handle on a /dev/net/tun device (pasta) or a connected Unix
socket (passt). However for the two modes we call different handler
functions. Simplify this a little by using different epoll types and
dispatching directly to the correct handler function.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
tap_handler() actually handles events on three different types of object:
the /dev/tap character device (pasta), a connected Unix domain socket
(passt) or a listening Unix domain socket (passt).
The last, in particular, really has no handling in common with the others,
so split it into its own epoll type and directly dispatch to the relevant
handler from the top level.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
epoll_ref contains a variety of information useful when handling epoll
events on our sockets, and we place it in the epoll_event data field
returned by epoll. However, for a few other things we use the 'fd' field
in the standard union of types for that data field.
This actually introduces a bug which is vanishingly unlikely to hit in
practice, but very nasty if it ever did: theoretically if we had a very
large file descriptor number for fd_tap or fd_tap_listen it could overflow
into bits that overlap with the 'proto' field in epoll_ref. With some
very bad luck this could mean that we mistakenly think an event on a
regular socket is an event on fd_tap or fd_tap_listen.
More practically, using different (but overlapping) fields of the
epoll_data means we can't unify dispatch for the various different objects
in the epoll. Therefore use the same epoll_ref as the data for the tap
fds and the netns quit fd, adding new fd type values to describe them.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We call tap_sock_reset() if tap_handler_passt() fails, or if we get an
error event on the socket. Fold that logic into tap_handler() passt itself
which simplifies the caller. It also makes it clearer that we had a
redundant EPOLL_CTL_DEL and close() in one of the reset paths, so fix that
too.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
If tap_handler_pasta() fails, we reset the connection. But in the case of
pasta the "reset" is just a fatal error. Fold the die() calls directly
into tap_handler_pasta() for simplicity.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We call tap_sock_unix_new() to handle a new connection to the qemu socket
if we get an EPOLLIN event on c->fd_tap_listen. If we get any other event
on the fd, we'll fall through to the "tap reset" path. But that won't do
anything relevant to the listening socket, it will just close the already
connected socket. Furthermore, the only other event we're subscribed to
for the listening socket is EPOLLRDHUP, which doesn't apply to a non
connected socket.
Remove EPOLLRDHUP from the subscribed events. We don't need to explicitly
add EPOLLERR, because errors are always reported. There's no obvious case
that would cause an error on a listening socket anyway, and it's not
obvious how we'd recover, treat it as a fatal error if it ever does happen.
Finally, fold all this handling into the tap_sock_unix_new() function,
there's no real reason to split it between there and tap_handler().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In tap_handler() if we get an error on the tap device or socket, we use
tap_sock_init() to re-initialise it. However, what we actually need for
this reset case has remarkably little in common with the case where we're
initialising for the first time:
* Re-initialising the packet pools is unnecessary
* The case of a passed in fd (--fd) isn't relevant
* We don't even call this for pasta mode
* We will never re-call tap_sock_unix_init() because we never clear
fd_tap_listen
In fact the only thing we do in tap_sock_init() relevant to the reset case
is to remove the fd from the epoll and close it... which isn't used in the
first initialisation case.
So make a new tap_sock_reset() function just for this case, and simplify
tap_sock_init() slightly as being used only for the first time case.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The number of items in pool_l4_t is defined to UIO_MAXIOV,
not TAP_SEQS. TAP_SEQS is the number of the sequences.
Fix the value used to compare seq->p.count with.
Fixes: 37c228ada8 ("tap, tcp, udp, icmp: Cut down on some oversized buffers")
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
[sbrivio: s/messages/sequences/ in commit message, extend
initialisation of packets in pool to UIO_MAXIOV items]
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
tap_ns_tun(), which runs in an ephemeral thread puts the fd it opens into
the global variable tun_ns_fd to communicate it back to the main thread
in tap_sock_tun_init().
However, the only thing tap_sock_tun_init() does with it is copies it to
c->fd_tap and everything else uses it from there. tap_ns_tun() already
has access to the context structure, so we might as well store the value
directly in there rather than having a global as an intermediate.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
There are several possible failure points in tap_ns_tun(), but if anything
goes wrong, we just set tun_ns_fd to -1 resulting in the same error
message.
Add more detailed error reporting to the various failure points. At the
same time, we know this is only called from tap_sock_tun_init() which will
terminate pasta if we fail, so we can simplify things a little because we
don't need to close() the fd on the failure paths.
Link: https://bugs.passt.top/show_bug.cgi?id=69
Link: https://github.com/containers/podman/issues/19428
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
ns_enter() returns an integer... but it's always zero. If we actually fail
the function doesn't return. Therefore it makes more sense for this to be
a function returning void, and we can remove the cases where we pointlessly
checked its return value.
In addition ns_enter() is usually called from an ephemeral thread created
by NS_CALL(). That means that the exit(EXIT_FAILURE) there usually won't
be reported (since NS_CALL() doesn't wait() for the thread). So, use die()
instead to print out some information in the unlikely event that our
setns() here does fail.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We don't handle defragmentation of IP packets coming from the tap side,
and we're unlikely to any time soon (with our large MTU, it's not useful
for practical use cases). Currently, however, we simply ignore the
fragmentation flags and treat fragments as though they were whole IP
packets. This isn't ideal and can lead to rather cryptic behaviour if we
do receive IP fragments.
Change the code to explicitly drop fragmented packets, and print a rate
limited warning if we do encounter them.
Link: https://bugs.passt.top/show_bug.cgi?id=62
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Since commit 0515adceaa ("passt, pasta: Namespace-based sandboxing,
defer seccomp policy application"), it makes no sense to close and
reopen the tap device on error: we don't have access to /dev/net/tun
after the initial setup phase.
If we hit ENOBUFS while writing (as reported: in one case because
the kernel actually ran out of memory, with another case under
investigation), or ENOSPC, we're supposed to drop whatever data we
were trying to send: there's no room for it.
Handle EINTR just like we handled EAGAIN/EWOULDBLOCK: there's no
particular reason why sending the same data should fail again.
Anything else I can think of would be an unrecoverable error: exit
with failure then.
While at it, drop a useless cast on the write() call: it takes a
const void * anyway.
Reported-by: Gianluca Stivan <me@yawnt.com>
Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu>
Fixes: 0515adceaa ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
When we receive packets from the tap side, we update the addr_seen fields
to reflect the last known address of the guest or ns. For ip4.addr_seen
we, sensibly, only update if the address we've just seen isn't 0 (0.0.0.0).
This case can occur during early DHCP transactions.
We have no equivalent case for IPv6. We're less likely to hit this,
because DHCPv6 uses link-local addresses, however we can see an source
address of :: with certain multicast operations. This can bite us if we
try to make an incoming connection very early after starting pasta with
--config-net: we may have only seen some of those multicast packets,
updated addr_seen to :: and not had any "real" packets to update it to a
global address. I've seen this with some of the avocado test conversions.
In any case, it can never make sense to update addr_seen to ::, so
explicitly exclude that case.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In practical terms, passt doesn't benefit from the additional
protection offered by the AGPL over the GPL, because it's not
suitable to be executed over a computer network.
Further, restricting the distribution under the version 3 of the GPL
wouldn't provide any practical advantage either, as long as the passt
codebase is concerned, and might cause unnecessary compatibility
dilemmas.
Change licensing terms to the GNU General Public License Version 2,
or any later version, with written permission from all current and
past contributors, namely: myself, David Gibson, Laine Stump, Andrea
Bolognani, Paul Holzinger, Richard W.M. Jones, Chris Kuhn, Florian
Weimer, Giuseppe Scrivano, Stefan Hajnoczi, and Vasiliy Ulyanov.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Commit 89e38f55 "treewide: Fix header includes to build with musl" added
extra #includes to work with musl. Unfortunately with the cppcheck version
I'm using (cppcheck-2.9-1.fc37.x86_64 in Fedora 37) this causes weird false
positives: specifically cppcheck seems to hit a #error in <bits/unistd.h>
complaining about including it directly instead of via <unistd.h> (which is
not something we're doing).
I have no idea why that would be happening; but I'm guessing it has to be
a bug in the cpp implementation in that cppcheck version. In any case,
it's possible to work around this by moving the include of <unistd.h>
before the include of <signal.h>. So, do that.
Fixes: 89e38f5540 ("treewide: Fix header includes to build with musl")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Roughly inspired from a patch by Chris Kuhn: fix up includes so that
we can build against musl: glibc is more lenient as headers generally
include a larger amount of other headers.
Compared to the original patch, I only included what was needed
directly in C files, instead of adding blanket includes in local
header files. It's a bit more involved, but more consistent with the
current (not ideal) situation.
Reported-by: Chris Kuhn <kuhnchris+github@kuhnchris.eu>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
If we define die() as a variadic macro, passing __VA_ARGS__ to err(),
and calling exit() outside err() itself, we can drop the workarounds
introduced in commit 36f0199f6e ("conf, tap: Silence two false
positive invalidFunctionArg from cppcheck").
Suggested-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
The newly introduced die() calls exit(), but cppcheck doesn't see it
and warns about possibly invalid arguments used after the check which
triggers die(). Add return statements to silence the warnings.
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The goto here really doesn't improve clarity or brevity at all. Use a
clearer construct.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In tap_send_frames() we send a number of frames to the tap device, then
also write them to the pcap capture file (if configured). However the tap
send can partially fail (short write()s or similar), meaning that some
of the requested frames weren't actually sent, but we still write those
frames to the capture file.
We do give a debug message in this case, but it's misleading to add frames
that we know weren't sent to the capture file. Rework to avoid this.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
