Most of out flow specific log messages are debug level for fear of flooding
the logs, even when they report real error conditions that might be off
significance.

Now that we have the mechanisms for log message rate limiting, we can do
better.  Promote many flow related messages to warning or error level, with
rate limiting.  While we're there add ratelimiting to a handful of existing
warning or error level messages.

They general heuristic is to promote messages that report a failure which
is not something that should be triggered by the guest doing something
weird.  This mostly means failures from socket operations we expect to be
legitimate.

Adding the ratelimiting means plumbing the 'now' timestamp through much
more of the code, hence the large churn.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Currently flow_log() and related functions / macros have a 'details'
parameter which indicates whether to add extra messages with details of the
flow's addresses.  This is still a bit awkward to invoke, and only used in
a few places.  Change the logic, to automatically include the details if
and only if the log priority is greater than LOG_DEBUG.

Rationale:

If at debug log level, there are already a bunch of debug messages tracking
the flow life cycle, which include those details (we make sure to retain
those).  It's usually pretty easy to cross reference a specific flow debug
message with the flow's history including the details.

If at higher log level, and we generate a flow-connected error or warning
we don't have those life cycle messages.  So, just giving the flow index
doesn't really tell you anything about which flow tripped the error.
Adding the address details make the error message significantly more
useful.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

flow.h has a collection of logging helpers that automatically include
information about a specific flow.  Which variants are present are a bit
ad-hoc, based on what we happened to want use (e.g. there are no
LOG_WARNING level versions, at present).  There's also a rather awkward
and only occasionally used flow_log_details_() helper to print additional
log messages with more details of the flow (basically its addresses).
It's particularly awkward to try to combine that with ratelimiting.

Re-organise this to be based around a flow_log__() internal helper, which
has bool parameters to include strerror() / perror information and/or
the extra details.  Add wrapper macros for all combinations of perror,
ratelimiting and DEBUG/WARNING/ERR priorities.

Be a little more consistent about parameter order between the various
functions / macros / wrappers while we're at it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

tcp_listen() and udp_listen() have only some simple logic around a call to
pif_listen(), and it's basically identical in each case.  If we move the
common logic into pif_listen() we can then remove {tcp,udp}_listen()
entirely, with their caller in fwd_sync_one() calling pif_listen()
directly.

We also move the logic converting from a protocol id to an epoll type from
fwd_sync_one() into pif_listen().  It's a bit arbitrary, but seems slightly
nicer that way.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Currently MAX_WINDOW is defined as a (signed) int, since '1' is a signed
literal.  This means that when it's used in the SEQ_*() macros, we're
making a signed / unsigned comparison.  While I don't think it was
incorrect in practice, confirming that requires reasoning though arcane
C type promotion rules.

Since it's obviously a non-negative value, change the constant to unsigned.
Change BUF_DISCARD_SIZE for the same reason, and because they're linked
via DISCARD_IOV_NUM, which must also be non-negative.  Change the types
of a few variables in tcp_prepare_iov() to match (otherwise we'd get some
compiler warnings).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Usually we use the SEQ_{LT,LE,GT,GE}() comparison macros on raw sequence
numbers, to see how they related handling possible wraparounds.  However,
in two cases we take a difference of sequence numbers then compare that
difference to 0.

As long as the difference is done using wrapping unsigned arithmetic,
that's not incorrect.  It's essentially doing comparisons in a "shifted"
space of sequence numbers, and I find working in both the raw and shifted
spaces makes it harder to reason about.  Specifically I sometimes find I
have to go through an additional layer of logic to convince myself that the
wraparound handling is correct.

In addition to confusing me, it also tends to confuse cppcheck, at least in
cppcheck 2.21.0 it often seems to trip false positives via cppcheck bug
14848.

It turns out that in each of the cases we use comparisons of sequence
differences against 0, it's pretty much just as clear - maybe clearer to
perform the comparisons against raw sequence numbers.  We still need the
difference afterwards, but it's much more obvious that it means what we
need it do, if we've already eliminated cases where the sequence numbers
aren't in the expected order.

In the case in tcp_data_from_tap() this also lets us simplify the diagram
explanation of the different cases a bit.  While there we correct the
diagram in the second discard case: seq was shown as partway into the
packet, which is not true by definition.  Instead seq_from_tap should be
some distance after the end of the packet.

Link: https://trac.cppcheck.net/ticket/14848
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

tcp_buf_data_from_sock() and tcp_vu_data_from_sock() start with some
identical logic to compute the amount of data from the socket that has
already been sent to tap but not acked.  Replace it with a single copy in
their common caller tcp_data_from_sock(), passing the already_sent amount
in as a parameter.

This does mean that we now execute this logic in the VU case even if the
queue is not active.  The logic is still valid in this case, and we don't
need to worry too much about wasting cycles in this edge case.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Since 260075bde769 ("tcp, udp, fwd: Run all port scanning from a
single timer"), CALL_PROTO_HANDLER() has only one user (tcp), so
inline it at the call site and remove the macro.

Merge tcp_timer() into tcp_defer_handler(), moving the timer interval
check there, matching the pattern used by flow_defer_handler() and
fwd_scan_ports_timer().

The weak declaration and null check for tcp_defer_handler are also
dropped as the function is always defined.

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>

tcp_listen_handler() has several error paths that will cancel the creation
of a new flow, after having accept()ed an incoming socket connection.
Coverity pointed out that in those cases we leak the new socket.  Correct
this by properly closing the socket.  Make sure to also set SO_LINGER so
that the peer will get an RST.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Both spliced and non-spliced TCP in some cases set the SO_LINGER socket
option in order to to force a TCP RST on a socket side connection.  In each
case we open code the setsockopt() logic.  We're shortly going to add
another place that needs this, so move the setsockopt() and error handling
logic into a shared helper.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>