It doesn't scale to export all addresses/routes on D-Bus as properties.
In particular not combined with PropertiesChanged signal. On a busy
system, this causes severe performance issues. It also doesn't seem very
useful. Routes and addresses are complex things (e.g. policy routing).
If you want to do anything serious, you must check netlink (or find
another way to get the information).
Note that NMPlatform already ignores routes of certain protocols
(ip_route_is_alive()). It also does not expose most route attributes,
making the output only useful for very limited cases (e.g. displaying to
the user for information).
Limit the number of exported entries to 100.
Try adding 100K routes one-by-one. Run a `nmcli monitor` instance.
Re-nice the nmcli process and/or keep the CPUs busy. Then start a script
that adds 100k routes. Observe. Glib's D-Bus worker thread receives the
messages and queues them for the main thread. The main thread is too
slow to process them, the memory consumption grows very quickly in Giga
bytes. Afterwards, the memory also is not returned to the operation
system, either because of fragmentation or because the libc allocator
does anyway not return heap memory.
It doesn't work to expose an unlimited number of objects on D-Bus. At
least not with an API, that sends the full list of all routes, whenever
a route changes. Nobody can use that feature either, because the only
use is a quick overview in `nmcli` output or a GUI. If you see 100+
routes there, that becomes unmanageable anyway. Instead use netlink if
you want to handle the full list of addresses/routes (or some other
API).
It doesn't scale. If you add 100k routes one-by-one, then upon each
change from platform, we will send the growing list of the routes on
D-Bus.
That is too expensive. Especially, if you imagine that the receiving end
is a NMClient instance. There is a D-Bus worker thread that queues the
received GVariant messages, while the main thread may not be able to
process them fast enough. In that case, the memory keeps growing very
fast and due to fragmentation it is not freed.
Instead, rate limit updates to 3 per second.
Note that the receive buffer of the netlink socket can fill up and we
loose messages. Therefore, already on the lowest level, we may miss
addresses/routes. Next, on top of NMPlatform, NMIPConfig listens to
NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE events. Thereby it
further will miss intermediate state (e.g. a route that exists only for
a short moment).
Now adding another delay and rate limiting on top of that, does not make
that fundamentally different, we anyway didn't get all intermediate states
from netlink. We may miss addresses/routes that only exist for a short
amount of time. This makes "the problem" worse, but not fundamentally
new.
We can only get a (correct) settled state, after all events are
processed. And we never know, whether there isn't the next event
just waiting to be received.
Rate limiting is important to not overwhelm D-Bus clients. In reality,
none of the users really need this information, because it's also
incomplete. Users who really need to know addresses/routes should use
netlink or find another way (a way that scales and where they explicitly
request this information).
If you add a large number of addresses/routes, then the output of
`nmcli` is unusable. It also doesn't seem too useful.
Limit the number to show up to 10 addresses and 10 routes.
If there are more than 10 addresses, then print an 11th line with
inet4 ... N more
Actually, if there are exactly 11 addresses, then don't waste an extra
line to print "1 more". Instead, still print the 11th address. Same for
routes.
This was forgotten to implement. But we cannot just forget about it.
Libnm emits a warning about unknown properties, exactly to catch such
bugs. Properties that are not implemented, must be marked to be ignored.
Next, support for this property will be added. But that introduces new
API, which cannot be backported. Hence, first fix the problem by marking
the property as ignored. This is a backportable change.
$ LIBNM_CLIENT_DEBUG="warning" G_DEBUG=fatal-warnings nmcli
(process:270215): nm-WARNING **: 15:22:56.125: libnm-dbus: <warn > nmclient[8094a8c217aae461]: get-managed-objects: [/org/freedesktop/NetworkManager/Devices/5]: ignore unknown property org.freedesktop.NetworkManager.Device.IPTunnel.FwMark
Trace/breakpoint trap (core dumped)
Fixes: 351c562491 ('devices: support VTI tunnels')
This commit fixes the build process for the documentation that was previously
unable to build separately via meson due to a dependency issue.
Previously, trying to build the API documentation via `ninja NetworkManager-doc`
failed due to missing dependencies (for example, `nm-dbus-types.xml` was not built).
I believe this happens due to some different handling of static paths vs. custom_target
by meson in this case.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1801
Fixes: 03637ad8b5 ('build: add initial support for meson build system')
For most strv or string properties, we cannot distinguish between
NULL/unset/default and empty.
It would be difficult to enter in nmcli or grasp how it differs. There
are probably many bugs, where we accept empty strings, and fail to
handle them correctly.
Anyway. For most strv arrays, and empty array and NULL/unset/default are
treated the same. That means, g_object_get() tends to always return NULL
(never an empty strv array) and g_object_set() of an empty strv array
will internally leave the GArray at NULL.
For a few properties, there is a difference. See "ipv[46].dns-options".
See also "clear_emptyunset_fcn" hook in libnm-setting.
Add a way to handle such strv properties with the "direct" mechanism.
Unfortunately, there are several possibilities how to handle NULL and
empty arrays. Therefore we have different variants.
Clean this up, and add a way to preserve whether the array is empty
(previous variants could not distinguish that).
Functions are also renamed, so that if you backport a user of the new
API, you'll get a compiler error if this patch is missing.
Also, nm_strvarray_get_strv_notnull() no longer takes a pointer to a
"GArray*". Previously, it used that to fake an empty strv array. Now
this returns NM_STRV_EMPTY_CC().
_nm_utils_dns_option_validate() allows specifying the address family,
and filters based on that. Note that all options are valid for IPv6,
but some are not valid for IPv4.
It's not obvious, that such filtering is only performed if
"option_descs" argument is provied. Otherwise, the "ipv6" argument is
ignored.
Regardless, it's also confusing to have a boolean "ipv6". When most
callers don't want a filtering based on the address family. They
actually don't want any filtering at all, as they don't pass an
"option_descs". At the same time passing a TRUE/FALSE "ipv6" is
redundant and ignored. It should be possible, to explicitly not select
an address family (as it's ignored anyway).
Instead, make the "gboolean ipv6" argument an "int addr_family".
Selecting AF_UNSPEC means clearly to accept any address family.
When connection down is explicitly called on the controller, the port
connection should also be deactivated with the reason user-requested,
otherwise any following connection update on the controller profile
will unblock the port connection and unnessarily make the port to
autoconnet again.
Fixes: 645a1bb0ef ('core: unblock autoconnect when master profile changes')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager-ci/-/merge_requests/1568
Add a new "stable-ssid" mode that generates the MAC address based on the
Wi-Fi's SSID.
Note that this gives the same MAC address as setting
connection.stable-id="${NETWORK_SSID}"
wifi.cloned-mac-address="stable"
The difference is that changing the stable ID of a profile also affects
"ipv6.addr-gen-mode=stable-privacy" and other settings.
For Wi-Fi profiles, this will encode the SSID in the stable-id.
For other profiles, this encodes the connection UUID (but the SSID and
the UUID will always result in distinct stable IDs).
Also escape the SSID, so that the generated stable-id is always valid
UTF-8.
We have many properties, and we aim that they have a small set of
"types". The purpose is that we can treat similar properties (with the
same type) alike.
One type are "direct" strv properties. Those still require some
C functions, like get-length(), clear(), add(), get-at-index().
The implementation of those functions should also be similar, so that
strv properties behave similar.
For that, make use of helper functions, so that little duplicate logic
is there.
Use some new nm_strvarray_*() functions, and unify/cleanup some code.
All related to strv properties in NMSetting classes.
The check "sizeof(const char *const *) ==
g_array_get_element_size((GArray *) strv)" is wrong, but probably
harmless, because most likely on our supported architectures all pointer
sizes are the same size.
Also, just use `sizeof(char *)` instead of `sizeof(const char *)`. Not
that it matters, but the GArray holds pointers of `char *`.
Also, consistently place the "sizeof()" on the left side of the
comparison.
There are various properties related to EEE, that we might want to add
support for in the future (for example, "ethtool.eee-advertise").
Don't use up the base name "eee", instead make it "eee-enabled". All
properties should have different prefixes, and "ethtool.eee" would be a
prefix of "ethtool.eee-advertise".
Also, the #define is already called NM_ETHTOOL_OPTNAME_EEE_ENABLED. This
also should be consistent.
Rename.
Fixes: 3165d9a2de ('ethtool: introduce EEE support')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1792
Previously, we first sort by the device's state, then by the active
connection's state. Contrast to `nmcli connection`, which first sorts
by the active connection's state.
It means, the sort order is somewhat different. Fix that.
In most cases, that shouldn't make a difference, because the
device's state and the active-connection's state should
correspond. However, it matters as we now treat external activations
different, and that is tied to the active connection.
CMP() is a confusing pattern. Sure enough, the sort order was wrong, for
example, `nmcli connection` would show
$ nmcli -f STATE,UUID,DEVICE c
STATE UUID DEVICE
activating 3098c902-c59c-45f4-9e5a-e4cdb79cfe1b nm-bond
activated e4fc23ac-54ab-4b1a-932a-ebed12c96d9b eth1
("activating" shown before "activated").
With `nmcli device`, we sort with compare_devices(). This first sorts by
device state (with "connected" being sorted first). Only when the device
state is equal, we fallback to nmc_active_connection_cmp(). So with
`nmcli device` we usually get "connected" devices first, and we don't
really notice that there is a problem with nmc_active_connection_cmp().
On the other hand, `nmcli connection` likes to sort first via
nmc_active_connection_cmp(), which gets it wrong. Profiles in
"activating" state are sorted first. That's inconsistent with `nmcli
device`, but it's also not what is intended.
Fix that.
Note the change in the test output. Both eth1 and eth0 are connected to
to the same profile, but one "eth0" the active-connection's state is
DEACTIVATING, while on "eth1" it's ACTIVATED (but both device's states
are "CONNECTED"). That's why "eth1" is now sorted first (as desired).
Fixes: a1b25a47b0 ('cli: rework printing of `nmcli connection` for multiple active connections')
- test for "-order" option with `nmcli connection show`.
- test for order of activated devices. Optimally, the devices
should be in activating vs. activated state. I fail to do that,
the mock implementation is cumbersome to use. It still seems useful
to have this (maybe it could be improved).
NM variants:
- evaluate arguments only once
- have a static assertion that the signedness of the argument agrees.
Like MIN()/MAX(), NM_MIN()/NM_MAX() now also evaluate to a constant
expression, if the arguments are already constant. That means, the only
reason why MIN()/MAX() was preferable over NM_MIN()/NM_MAX() is no
longer relevant. Except there are a few places where NM_MIN()/NM_MAX()
cannot be used. In those places use NM_MIN_CONST()/NM_MAX_CONST().
glib's MIN()/MAX() will be replaced by NM_MIN()/NM_MAX().
There are however a few places where NM_MIN()/NM_MAX() cannot
be used.
Adjust those places to use NM_MIN_CONST()/NM_MAX_CONST() instead.
Glib's MIN()/MAX() should not be used, in favor of NM_MIN()/NM_MAX().
That's because the NM variants
- evaluate arguments only once
- have a static assertion that the signedness of the arguments matches
However, previously those macros never evaluated to a compile time
constant. Unlike the glib variants, which do so when the arguments are
compile time constants. That is sometimes important when using the
macros in a context that requires a constant.
Extend NM_MIN()/NM_MAX() to be a compile time constant, when possible.
Note that there are still a few places where NM_MIN()/NM_MAX() cannot be
used due to the expression statement. For those cases, there is
NM_MIN_CONST()/NM_MAX_CONST().
Comparing integers of different signedness gives often unexpected
results. Adjust usages of MIN()/MAX() to ensure that the arguments agree
in signedness.
nm_hash_siphash42() uses a randomized seed like nm_hash*(). In this case,
we want to always generate the same fake timestamp, based on the host-id.
In practice, it doesn't really matter, because this is only the fallback
path for something gone horribly wrong already.
