In practice there is little difference.
Previously, "strbuf" would own the string until the end of the function,
when the "nm_auto_str_buf" cleanup attribute destroys it. In the
meantime, we would pass it on to _fw_nft_call_sync(), which in fact
won't access the string after returning.
Instead, we can just transfer ownership to the GBytes instance. That seems
more logical and safer than aliasing the buffer owned by NMStrBuf with
a g_bytes_new_static(). That way, we don't add a non-obvious restriction
on the lifetime of the string. The lifetime is now guarded by the GBytes
instance, which, could be referenced and kept alive longer.
There is also no runtime/memory overhead in doing this.
At some places we scheduled a timeout in NM_SHUTDOWN_TIMEOUT_MAX_MSEC.
There, we want to make sure that we don't take longer than
NM_SHUTDOWN_TIMEOUT_MAX_MSEC. But this leaves the actual wait time
unspecified.
Those callers don't want to wait an undefined time. They really should
be clear about how long they wait. Hence, use NM_SHUTDOWN_TIMEOUT_1500_MSEC
which makes it clear this is 1500 msec but also chosen to be not longer than
NM_SHUTDOWN_TIMEOUT_MAX_MSEC.
The abbreviations "ms", "us", "ns" don't look good.
Spell out to "msec", "usec", "nsec" as done at other places.
Also, rename NM_SHUTDOWN_TIMEOUT_MS_WATCHDOG to
NM_SHUTDOWN_TIMEOUT_ADDITIONAL_MSEC.
Also, rename NM_SHUTDOWN_TIMEOUT_MS to NM_SHUTDOWN_TIMEOUT_MAX_MSEC.
There are different timeouts, and this is the maximum gracetime we
will give during shutdown to complete async operations.
Naming is hard, but I think these are better names.
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
During timeout we cancel the (internal) GCancellable. But the overall
error reason is not cancellation by the user, it's timeout. Fix
the error reason.
Fixes: 9ebdb967de ('firewall: implement masquerading for shared mode with nftables')
(cherry picked from commit 897c6a5744)
It seems that the nftables backend works well. Let's change the default.
This will also be backported to nm-1-32 branch, for 1.32.0 release.
This reverts commit 0609f1f31c.
(cherry picked from commit 79d9441567)
ntables backend is not yet well tested. Don't flip the default yet
but for now always use iptables.
Once nftables is shown to work well, revert this patch.
Add support for nftables, as a second backend beside iptables (firewalld
still missing).
Like iptables, choose to call the `nft` tool. The alternative would be
to use libnftables or talk netlink.
It's ugly to blocking wait for a process to complete. We already do that
for iptables, but we better should not because we should not treat other
processes as trusted and not allow untrusted code to block NetworkManager.
Fixing that would require a central manager that serializes all requests.
Especially with firewalld support, this will be interesting again,
because we don't want to synchronously talk D-Bus either.
For now, `nft` is still called synchronously. However, the internal
implementation uses an asynchronous function. That currently
serves no purpose except supporting a timeout. Otherwise, the only
reason why this is asynchronous is that I implemented this first, and
I think in the future we want this code to be non-blocking. So, instead
of dropping the asynchronous code, I wrap it in a synchronous function
for now.
The configured nft table is:
table inet nm-shared-eth0 {
chain nat_postrouting {
type nat hook postrouting priority srcnat; policy accept;
ip saddr 192.168.42.0/24 ip daddr != 192.168.42.0/24 masquerade
}
chain filter_forward {
type filter hook forward priority filter; policy accept;
ip daddr 192.168.42.0/24 oifname "eth0" ct state { established, related } accept
ip saddr 192.168.42.0/24 iifname "eth0" accept
iifname "eth0" oifname "eth0" accept
iifname "eth0" reject
oifname "eth0" reject
}
}
The new name makes it more generic, because the limitation exists
for iptables chains. Everything else (iptables comments, nftables
tables) has no such length limit.
It's still not a very good name, but it seems better then
NMUtilsShareRules.
Currently, NMFirewallConfig is mostly about masquerading for shared
mode. But in practice, it's a piece of configuration for something to
configure in the firewall (the NAT and filter rules).
