Currently, NMDeviceWireguard does neither set connection_type_check_compatible
nor implement check_connection_compatible. That means, it appears to be compatible
with every connection profile, which is obviously wrong.
Allow devices not to implement check_connection_compatible() and avoid the issue
by rejecting profiles by default.
Ok, I changed my mind.
The new behavior seems to make more sense to me. Not that it matters,
because we always use nm_utils_strbuf*() API with buffers that we expect
to be large enough to contain the result. And when truncation occurs,
we usually don't care much about it. That is, there is no code that
uses nm_utils_strbuf*() API and handles string truncation in particular.
NM_IN_SET will only compare string pointers and isn't useful for
checking if nm_setting_wireless_get_mode (s_wifi) is infrastructure.
Fixes: 570e1fa75b
Make sure we free our IWD agent objects whenever we're freeing the
IWD Object Manager. We're registering those objects on the same DBus
connection as the Object Manager so that they're visible to IWD, and
our only reference to that connection is through priv->object_manager
so even though the connection isn't changing when we free the object
manager and create a new one, we still need to free the agent object.
We could maybe keep a reference to the connection, but I'm not sure
there's any warranty that it doesn't get closed. We could also use
nm_dbus_manager_get_connection (nm_dbus_manager_get ()) and only
register and free the agent once, since it happens to be the same
connection but it'd perhaps be a hack to rely on this.
- previously, parsing wireguard genl data resulted in memory corruption:
- _wireguard_update_from_allowedips_nla() takes pointers to
allowedip = &g_array_index (buf->allowedips, NMWireGuardAllowedIP, buf->allowedips->len - 1);
but resizing the GArray will invalidate this pointer. This happens
when there are multiple allowed-ips to parse.
- there was some confusion who owned the allowedips pointers.
_wireguard_peers_cpy() and _vt_cmd_obj_dispose_lnk_wireguard()
assumed each peer owned their own chunk, but _wireguard_get_link_properties()
would not duplicate the memory properly.
- rework memory handling for allowed_ips. Now, the NMPObjectLnkWireGuard
keeps a pointer _allowed_ips_buf. This buffer contains the instances for
all peers.
The parsing of the netlink message is the complicated part, because
we don't know upfront how many peers/allowed-ips we receive. During
construction, the tracking of peers/allowed-ips is complicated,
via a CList/GArray. At the end of that, we prettify the data
representation and put everything into two buffers. That is more
efficient and simpler for user afterwards. This moves complexity
to the way how the object is created, vs. how it is used later.
- ensure that we nm_explicit_bzero() private-key and preshared-key. However,
that only works to a certain point, because our netlink library does not
ensure that no data is leaked.
- don't use a "struct sockaddr" union for the peer's endpoint. Instead,
use a combintation of endpoint_family, endpoint_port, and
endpoint_addr.
- a lot of refactoring.
Move NMLinuxPlatformPrivate earlier.
In the past, I moved the declaration of NMLinuxPlatformPrivate
after utility functions which are independent from platform
instance.
However, parsing netlink messages actually requires
NMLinuxPlatformPrivate, because we want to access the "genl"
socket.
So, move the types to the beginning of the file, like we do
for most other source files.
The _lookup_cached_link() function, should not skip over links which are
currently in the cache, but not in netlink. Instead, let the callers
skip them, as they see fit.
No change in behavior, because the few callers now explicitly check
for this.
- drop "goto error_label" in favor of returning right away.
At most places, there was no need to do any cleanup or
the cleanup is handled via nm_auto().
- adjust the return types of wireguard functions to return
a boolean success/failure, instead of some error code which
we didn't use.
- the change to _wireguard_get_link_properties() is intentional.
This was wrong previously, because in _wireguard_get_link_properties()
obj is always a newly created instance, and never has a genl
family ID set. This will be improved later.
It's only used internally, and it seems not very useful to have.
As it is confusing to have multiple functions for doing something
similar, drop it -- since it's not really used. I also cannot imagine
a good use-case for it.
When we print info about the link, we also want to print
info about the referenced lnk instance, which commonly contains
link-specific data.
For NMP_OBJECT_TO_STRING_PUBLIC this was done correctly, by
calling to-string of public fields on the lnk object.
For NMP_OBJECT_TO_STRING_ALL, we wrongly just delegated to the
public to-string function, this means, for the lnk object we
would not print all fields.
Fix that.
The DNS manager reacts to NM_DEVICE_IP4_CONFIG_CHANGED events, which
are generated when there is a relevant change to an IP4 configuration.
Until now, changes to the mdns,llmnr properties were not
considered relevant (and neither minor, this is already a bug).
Promote them to relevant so that the DNS manager is notified and will
rewrite the DNS configuration when one of this properties changes.
Note that the DNS priority should be considered relevant and added
into the checksum as well, but is a problem right now because in the
DNS manager we rely on the fact that an empty configuration (i.e. just
created) has a zero checksum. This is needed to avoid rewriting
resolv.conf when there is no change. The DNS priority initial value
depends on the connection type (VPN or not), so it's a bit difficult
to add it to checksum maintaining the assumption of checksum(empty)=0.
This should be improved in the future.
Does not address the issues that the existing logging is much too verbose
and does not provide necessary context for what it's complaining. The
logging messages should be improved.
At least, the logging macro gives all messages a "ifupdown: " prefix.
Don't listen to udev to find out about devices. First of all, using udev
for this is already very wrong, because we have the platform cache.
Anyway, all that the device information is used, is pointless as well.
Drop it.
It's pointless because:
The entires in eni_ifaces are already indexed by the interface name.
Likewise, all NMIfupdownConnection set "connection.interface-name" to
restict the profile by name.
/e/n/i matches devices is by name, that's it. We don't need udev to
look up the MAC address (by name!!) to later ignore/match devices
by MAC address. Especially, because NetworkMaanger can already
restrict profiles to devices based on the interface name.
Likewise, NetworkMaanger can use the interface name for the
unmanaged-specs.
It's wrong to extend the on-disk configuration from /e/n/i with runtime
information from udev, especially, because other NetworkMaanger layers
are perfectly content using the interface name for this purpose.
Also, bind_device_to_connection() was fundamentally wrong. It's wrong
to modify the settings connection at random moments (on udev event).
If at all, that should only happen during connection load/reload.
This may have been necessary a long time ago, when unmanaged devices were
not expressed by device-match-specs, but by the HAL UDI. That was since
improved, for example by commit c9067d8fed.
The "connections" hash contains a mapping of block->name (iface) to the
NMSettingsConnection. The "eni_ifaces" hash contains the name of all
blocks which are mentioned, but for which no connection was created.
Merge the two hashes. We don't need to keep track of whether a
connections was successfully created ("connections"), but the
same name also has a non-connection block. This information is
unnecessary, so one hash is enough.
@unmanage_well_known directly depends on the "ifupdown.managed" setting from
NetworkManager.conf. Rename it (and invert the meaning) so that this
relation ship becomes clearer.
Also, the double negation of "if (!unmanaged_well_known)" hurts the
brain.
This is only useful for testing. But since the managed flag is configurable
via NetworkManager.conf, there is no point in having a define as well. If
you want to test it, just configure it.
And if you really want to patch the code, then patch
"priv->unmanage_well_known" to be always TRUE.
- use _NMLOG() macro and give logging message a sensible prefix
- downgrade logging severity. Most of these messages are not
important to warrant <info> or <warn> level.
- the logging is generally rather bad. Messages like
"bind-to-connection: locking wired connection setting"
don't indicate which profile is locked to which MAC address.
TODO.
- use gs_unref_hashtable for managing lifetime
- only allocate the hashtable if necessary, and use g_hash_table_add()
which is optimized by HashTable.
- actually copy the block->name that is used as key. While not
necessary at the moment, it is very ugly how ifparser_getfirst()
returns static data. Optimally, this would be fixed and we create
and destroy the parser results. Hence, ensure the lifetime of
the key.
- initialize the hash tables in the plugins constructor, not during
initialize().
- let all dictionaries own a copy/reference of the keys and values, and
properly free them when the values are removed. In general, avoid
leaks by properly managing lifetimes.
- in @eni_ifaces, don't add a pointless dummy value "known". It has
overhead for no benefit.
NMSettingsPlugin was a glib interface, not a regular GObject
instance. Accordingly, settings plugins would implement this interface
instead of subclassing a parent type.
Refactor the code, and make NMSettingsPlugin a GObject type. Plugins
are now required to subclass this type.
Glib interfaces are more cumbersome than helpful. At least, unless
there is a good reason for using them.
Our settings plugins are all internal API and are entirely under
our control. It also means, this change is fine, as there are no
implementations outside of this source tree.
Using interfaces do would allow more flexibility in implementing the
settings plugin.
For example, the plugin would be able to derive from any other GObject
type, like NMKimchiRefrigerator. But why would we even? Let's not add monster
classes that implement house appliances beside NMSettingsPluginInterface.
The settings plugin should have one purpose only: being a settings plugin.
Hence, requiring it to subclass NMSettingsPlugin is more than resonable. We
don't need interfaces for this.
Now that NMSettingsPlugin is a regular object instance, it may also have
state, and potentially could provide common functionality for the plugin
implementation -- if that turns out to be useful. Arguably, an interface can
have state too, for example by attaching the state somewhere else (like
NMConnection does). But let's just say no.
On a minor note, this also avoids some tiny overhead that comes with
glib interfaces.
