For historic reasons is NMSettingBond implemented differently from other
settings. It uses a strdict, and adds some validation on top of that.
The idea was probably to be able to treat bond options more generically.
But in practice we cannot treat them as opaque values, but need to know,
validate and understand all the options. Thus, this implementation with a
strdict is not nice.
The user can set the GObject property NM_SETTING_BOND_OPTIONS to any
strdict, and the setter performs no validation or normalization. That
is probably good, because g_object_set() cannot return an error to
signalize invalid settings. As often, we have corresponding C API like
nm_setting_bond_add_option() and nm_setting_bond_remove_option(). It
should be possible to get the same result both with the C API and with
the GObject property setting. Since there is already a way to set
certain invalid values, it does not help if the C API tries to prevent
that. That implies, that also add-option does not perform additional
validation and sets whatever the user asks.
Remove all validation from nm_setting_bond_add_option() and
nm_setting_bond_remove_option(). This validation was anyway only very
basic. It was calling nm_setting_bond_validate_option(), which can check
whether the string is (for example) and integer, but it cannot do
validation beyond one option. In most cases, the validation needs to
take into account the bond mode or other options, so validating one
option in isolation is not very useful.
Proper validation should instead be done via nm_connection_verify().
However, due to another historic oddity, that verification is very
forgiving too and doesn't reject many invalid settings when it should.
That is hard to fix, because making validation more strict can break
existing (and working) configurations. However, verify() already contains
basic validation via nm_setting_bond_validate_option(). So in the previous
behavior nm_setting_bond_add_option() would silently do nothing (only
returning %FALSE) for invalid options, while now it would add the
invalid options to the dictionary -- only to have it later fail validation
during nm_connection_verify(). That is a slight change in behavior, however it
seems preferable.
It seems preferable and acceptable because most users that call
nm_setting_bond_add_option() already understand the meaning and valid
values. Keyfile and ifcfg-rh readers are the few exceptions, which really just
parse a string dictionary, without need to understand them. But nmtui
or nmstate already know the option they want to set. They don't expect
a failure there, nor do they need the validation.
Note that this change in behavior could be dangerous for example for the
keyfile/ifcfg-rh readers, which silently ignored errors before. We
don't want them to start failing if they read invalid options from a
file, so instead let those callers explicitly pre-validate the value
and log an warning.
https://bugzilla.redhat.com/show_bug.cgi?id=1887523
If the target hidden network is already recorded by IWD with its SSID
during a previous active scan, use the Network.Connect() API instead of
Station.ConnectHiddenNetwork() which would fail in IWD version up to
1.9. This is a rare corner case scenario though.
Also drop the !nm_wifi_ap_get_supplicant_path(ap) check, I'm not
sure when if ever that condition can be true, more so now that we're
checking nm_wifi_ap_get_fake(ap) before that.
Until now we didn't rely on InterfacesAdded and InterfacesRemoved
signals for tracking when IWD finds new Wi-Fi networks or expires
networks not seen in the latest scans. Instead we'd request the whole
list of networks currently seen by IWD every time the Station.Scanning
property would go from true to false. However the
Station.GetOrderedNetworks() IWD method that we use has a deficiency
up until 1.9 (I plan to fix it soon) where it won't show the hidden
network discovered in the course of the last ConnectHiddenNetwork() call
if that call was unsuccessful, in other words where the new network has
not been saved as a Known Network. A new ConnectHiddenNetwork() will
fail with the "NotHidden" error, so we have to use the Network.Connect()
call for such a network but to find it out we need to track the
InterfacesAdded signals. Doing this may also improve autoconnect speed
in some cases so overall I think it's a good idea.
When IWD asks us for a secret check that we're in NM_DEVICE_STATE_CONFIG
and not for example already in NM_DEVICE_STATE_NEED_AUTH. I believe that
should only happen if IWD is aborting the previous connection attempt and
connecting to a different network due to a timeout or due to somebody
outside NM calling Connect() on an IWD network object...
Guessing what IWD is doing this way is a bit fragile in the long term
but we have to do that as long as we want to override IWD's internal
autoconnect, which I guess we may be able to stop doing at some point.
IWD's Station.State property remains at "connect" or "disconnected"
while IWD is waiting for secrets for a new conncetion, so if we want to
scan only when NM might be in auto-connect (which was the goal) we need
to also look at NMDevice's state. We want to scan whenever wifi is
disconnected and there's no active connection request, which is the same
as saying whever priv->current_ap is unset so for simplicity look at
priv->current_ap. Also in schedule_periodic_scan() don't check whether
Station.State is "disconnected" because priv->can_scan is equivalent to
Station.State being one of ("disconnected", "connected").
Hidden networks are supported in the iwd backend since 1.24.0 but some
places in the code have not been updated to reflect this.
In check_connection_available copy the hidden network check and
corresponding comment from the wpa_supplicant backend. In
act_stage1_prepare drop a straight "hidden networks are unsupported"
comment and a check -- fortunately this check happened to be ineffective
because @mode was more often NULL than NM_SETTING_WIRELESS_MODE_INFRA so
nm_streq0 was not enough. Update comments elsewhere.
There's still one of two corner cases where the user-experience will not
be perfect for hidden networks due to iwd limitations, I'll try to work
around them in another commit.
I first noticed a format string with missing parameters and then that
the compiler wasn't complaining and that's because
nm_utils_error_set_literal doesn't take a format string.
When a connection fails and IWD returns net.connman.iwd.Aborted, we know
whether the abort is caused by us cancelling a secrets request so use
tha knowledge to decide whether to use the NM_DEVICE_STATE_REASON_NO_SECRETS
reason code for the state switch to NM_DEVICE_STATE_FAILED.
Use the NM_UTILS_ERROR_CONNECTION_AVAILABLE_INCOMPATIBLE constant in
place of NM_UTILS_ERROR_CONNECTION_AVAILABLE_TEMPORARY more often in
check_connection_compatible as appropriate.
When a wifi device is in a bridge, the supplicant must be aware of it,
as a socket must be opened on the bridge to receive packets.
Set the BridgeIfname property of the supplicant Interface object
before starting the association. Note that the property was read-only
in the past and recently [1] became read-write. When using a
supplicant version without the patch, writing the property will return
an InvalidArgs error and NetworkManager will print a warning.
[1] https://w1.fi/cgit/hostap/commit/?id=1c58317f56e312576b6872440f125f794e45f991https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/83
The underscore somehow indicated that these would be an internal
function. Which they are in the sense that they are in "shared/nm-glib-aux/".
But they part of our internal helper functions, and in our code base
their use is no discouraged or "private.
Also, next I'll replace the function call with a macro, so, I will
have a need for the underscore name.
Rename.
- use a guint64 variable to avoid wrapping the counter
- cache the used ID in NMDevice. This way, the same NMDevice
instance will get the same UDI path when it realizes
and unrealizes multiple times.
GDBusObjectManagerClient's interface-added and interface-removed signals
are not emitted when the new interfaces are added to a completely new
object or the removal results in the object disappearing. In other
words one interface is never reported both through interface-added and
object-added (or -removed) signals. This kind of makes sense but isn't
documented explicitly so interface-added seemed to correspond to DBus
InterfacesAdded signals which it doesn't.
We need to watch for both kinds of signals and although most things
work without us receiving the signals at all, it causes some race
conditions. For example on hotplug, devices wouldn't transition to
"disconnected" if a device was discovered by NMManager before it
appeared on IWD's dbus interface because that scenario relied on the
dbus signal.
The automatic scanning every 20 seconds while connected has been
annoying users because of the extra connection latency, drop it. The
UIs are supposed to be requesting scans whenever an AP list update is
needed (?).
Fix a crash on device unplugging caused by keeping our signal handlers
for GDBusProxies connected after a call to dispose(). Do this by
replacing most cleanup steps by a nm_device_iwd_set_dbus_object(self, NULL)
call which is more meticulous.
Compare to the connection's GetSettings() call, which is not protected
by policykit permissions. It only checks that the requesting user is
allowed according to "connection.permission".
Previously, device's GetAppliedConnection() requires "network-control"
permissions. This although it only reads a profile, without modifying
anything. That seems unnecessary, also because in the common case the
applied connection is identical to the current settings connection, and
the latter can be read without special permissions.
Don't require a special policykit permission to read the applied
connection.
https://bugzilla.redhat.com/show_bug.cgi?id=1882380
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
Adding NML3Cfg support is a large effort that is done in parallel.
While already parts of the code is merged, it is not actually used
yet. Remove the parts from NMDevice that interact with NML3Cfg
before we actually start using it.
The point is that we might want to do a release before switching
over to the new way. For that release, we should not have the unused
code in NMDevice.
This patch will later be restored and extended.
Currently all NMDevice instance register to the platform change signals,
then if a signal for their IP ifindex appears, they schedule a task on
an idle handler. That is wasteful.
NML3Cfg already gets a notification on an idle handler and can just re-emit
it to the respective listeners.
With this, there is only one subscriber to the platform signals (NMNetns)
which then multiplexes the signals to the right NML3Cfg instances, and
further.
All our devices will return the same value on D-Bus: a "u" variant with zero value.
Since NMDBusObject caches all the property values, we can share the instance.
The "Ip4Address" property of "org.freedesktop.NetworkManager.Device"
interface is deprecated since version 0.9.9.1 (2013). Also, the property
is not exposed by libnm and generally not useful.
Drop the code to maintain it. The property still exists but always
returns 0 (0.0.0.0).
Previously, if we passed ra_timeout 0 to NMNDisc, then it would
calculate the effective timeout based on the router-solicitations
and the router-solicitation-interval.
The caller may want to know the used timeout, to also run its own timers
with the same timeout. Hence, it cannot leave this automatism internal
to NMNDisc.
Note that when NetworkManager tries to allocate more than 256 networks,
then previously the allocation would fail. We no longer fail, but log an
error and reuse the last address (10.42.255.1/24).
It's simpler to have code that cannot fail, because it's often hard to
handle failure properly. Also, if the user would configure two shared
profiles that explicitly use the same subnet, we also wouldn't fail. Why
not? Is that not a problem as well? If it is not, there is no need to
fail in this case. If it is a problem, then it would be much more
important to handle this case otherwise -- since it's more likely to
activate two profiles that accidentally use the same subnet than
activating 257+ shared profiles.
It is solely computed from the lease information (the GHashTable).
No need to pass it along as separate argument in NM_DHCP_CLIENT_SIGNAL_STATE_CHANGED,
especially since it only applies to IPv6.
It's important to find place in code where are field (state) gets
mutated. Make sys_iface_state field const, but add a mutable alias
via a union. You can now grep for places that change the field.
Setting "active_slave" fails unless the slave is currently present and
IFF_UP. That complicates the code, because we cannot set the property
at any time, but only under the right circumstances.
But really, "active_slave" option is something for debugging. It's not
an option which should be set by NetworkManager. The right option
instead is "primary", which will tell kernel to make the slave active,
when it is ready.
Deprecate the "active_slave" option and make it an alias for "primary".
https://bugzilla.redhat.com/show_bug.cgi?id=1856640
Code doesn't get simpler by having more functions -- if these functions
are only called once.
What actually is a problem is repeated, redundant code. Like the list of
bond options that can be reapplied. But the function didn't help to
avoid repeating the list.
Add a macro for the list of bond options we are going to set. By seeing
them side-by-side, it is hopefully simpler to see that all options are
specified correctly.
We see that:
- the *_SUBSET defines don't include the options that we are explicitly
setting, that is "mode", "active_slave" and "arp_ip_target".
- OPTIONS_REAPPLY_SUBSET contains 4 options less than OPTIONS_APPLY_SUBSET:
"ad_select", "ad_user_port_key", "lacp_rate" and "tlb_dynamic_lb".
These are the options that are marked as BOND_OPTFLAG_IFDOWN in
kernel.
