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.
Heavily rework NML3Cfg's ACD handling.
- the (user facing) API changed, so that we can ask the current ACD
state of an address with nm_l3cfg_get_acd_addr_info(). So, the
acd-event signal is only to notify when the state changes, it does
not carry information that you couldn't fetch anytime.
- add clearer ACD states (NML3AcdAddrState). The current (ACD) state
of an address is important and becomes part of the information that
we expose.
- add new ACD state "USED", when ACD fails. This blocks the address from
being used. Usually the caller would either remove the (used) address
or force reconfigure it (by setting acd_timeout_msec to zero).
- add new ACD state "CONFLICT". Previously conflicts were not handled.
Now the API allows to specify the defend policy. A conflicted address
also gets blocked from being used.
- add new ACD state "EXTERNAL_REMOVED". This happens when we have an
address we wanted to configure, but then the address is no longer
on the interface. For example because the user removed it from the
interface. This also leaves the device indefinitely blocked, and
is important to stop announcing the address.
- add a new ACD state "READY". This indicates that the address is ready
to be configured, but not yet actually configured on the device. This
is the step before "DEFENDING".
ACD is handled by NML3Cfg and it intercepts the IP addresses when
merging the NML3ConfigData.
Originally, I thought that in such a case, the merged l3cd instance
would simply not contain any addresses that ACD have still pending or
which have a conflict.
However, I think it's better (clearer and possibly useful), to still
merge such addresses, but flag them that they are ignored when syncing
the addresses to platform.
It is not yet used, but it will be used to mark instances that
are not supposed to be configured in platform, because ACD is
either still pending of failed.
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
It's cumbersome if we always need to cast our arguments for
the strv helper functions. Depending on the situation, we often
have a "char **" or a "const char *const*" argument.
Use NM_CAST_STRV_CC() macros instead. This macro uses C11's _Generic()
and casts types that are presumed to be safe. This tends to be less
typing and more type-safe, because you don't need an explicit C cast
(which would overrule any warning that the compiler might have for you).
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.