Before, ovsdb_call_method() has a long list of arguments
to account for all possible commands. That does not scale.
Instead, introduce a separate OvsdbMethodPayload type and
only add a macro to allow passing the right parameters.
The text should match the OvsdbCommand enum. If the enum
value is named OVSDB_ADD_INTERFACE, then we should print
"add-interface". Or alternatively, if you think spelling
out interface is too long, then the enum should be renamed.
I don't care, but name should correspond.
ovsdb sends monitor updates, with "new" and "old" values that indicate
whether this is an addition, and update, or a removal.
Since we also cache the entries, we might not agree with what ovsdb
says. E.g. if ovsdb says this is an update, but we didn't have the
interface in our cache, we should rather pretend that the interface
was added. Even if this possibly indicates some inconsistency between
what OVS says and what we have cached, we should make the best of it.
Rework the code. On update, we compare the result with our cache
and care less about the "new" / "old" values.
We will call the function directly as well. Lets aim to
get the types right.
Also the compiler would warn if the cast to (GDestroyNotify)
would be to a fundamtally different function signature.
GHashTable is optimized for data that has no separate value
pointer. We can use the OpenvswitchBridge structs as key themselves,
by having the id as first field of the structure and only use
g_hash_table_add().
Of course, in practice "mtu" is much smaller than 2^31, and
also is sizeof(int) >= sizeof(uint32_t) (on our systems). Hence,
this was correct. Still, it feels ugly to pass a unsigned integer
where not the entire range is covered.
- rename "id" to something more distinct: "call_id".
- consistently use guint64 type. We don't want nor need
to handle negative values. For CALL_ID_UNSPEC we can use
G_MAXUINT64.
- don't use "i" format string for the call id. That expects
an "int", so it's not clear how this was working correctly
previously. Also, "int" has a smaller range than our 64bits.
Use instead "json_int_t" and cast properly in the variadic
arguments of json_pack().
Also, never update the value to %NULL. If the current
message does not contain a UUID, keep the previous one.
Fixes: 830a5a14cb ('device: add support for OpenVSwitch devices')
"nm-device-logging.h" defines logging macros for a NMDevice instance.
It also expects a "self" variable in the call environment, and that
variable had to be in the type of NMDevice or the NMDevice subclass.
Extend the macro foo, so that @self can be either a NMDevice* pointer
or a NMDevice$SUBTYPE.
Of course, that would have always been possible, if we would simply cast
to "(NMDevice *)" where we need it. The trick is that the macro only
works if @self is one of the two expected types, and not some arbitrary
unrelated type.
Kernel does a auto-mtu adjusting process whenever a port is added/removed from
the bridge, this can cause issues when NM wants to explicitly set an MTU which is
equal to the bridge default one (1500) because if later a port is added with a
different MTU the kernel will assign the bridge that port's MTU resulting in the bridge
runtime configuration differing from the bridge's NM connection profile.
What we can do is to always apply the MTU manually for the bridge (if explicitly
set by the profile), after doing so the kernel won't modify the MTU anymore,
which is what we want, problem is that kernel won't actually apply the MTU
to the netdev if it's not actually changing so we first apply it to
MTU-1 and then to the desired value.
https://bugzilla.redhat.com/show_bug.cgi?id=1778590
Signed-off-by: Antonio Cardace <acardace@redhat.com>
- we commonly use "int addr_family" as parameters to functions.
But then inside the function, we often need to do something for
IPv4 or IPv6 specifically. Instead of having lots of redundant
"if (addr_family == AF_INET)" checks, prefer to have a variable
IS_IPv4 and/or use NM_IS_IPv4() macro.
- don't make the "IS_IPv4" variable a gboolean but an int. gboolean
is a typedef for int, so it's in practice exactly the same. However,
we use "IS_IPv4" as index to arrays of length 2, where at position
"1" we have the value related to IPv4. Using a gboolean to index
an array is a bit odd. Maybe a "int" is preferable here.
This is more about doing consistently one or the other. There are
no strong reasons to prefer gboolean or int.
Apparently it is not actually used, but the function implements
a return value for AF_UNSPEC, while also asserting that the addr_family
is AF_INET/AF_INET6. Drop the assertions.
These macros are consistent with NMP_OBJECT_TYPE_IP_ADDRESS()
and NMP_OBJECT_TYPE_IP_ROUTE(), in name and usage.
Replace the previous functions that had inconsistent and a verbose
naming.
It's a bit ugly that NMActRequest also tracks the shared rules.
Why? It's just some additional state (the rules) and some additional
actions that should be done when activating/deactivating the profile.
NMActRequest also doesn't track the NMDhcpClient, so why these shared
rules?
Also, removing the rules from an object destructor is ugly. NMActRequest
is a GObject and ref-counted. We should not make assumptions when the
last reference gets releases, at least not in cases like this, where
we hand out the reference and the object is passed around through large
parts of the source code.
For now, still let NMActRequest keep track of NMUtilsShareRules.
Later this will be refactored too.
A static const array is marked as immutable by the linker.
This is what we want, because there is no need to change this
array.
Also, the tailing %NULL entry is not necessary, we can just
iterate over the fixed number of elements.
For hidden networks, we usually don't have an SSID. We try to match
and fill the SSID based on the profiles that we have:
<debug> [1603798852.9918] device[6b383dca267b6878] (wlp2s0): matched hidden AP AA:BB:CC:DD:EE:FF => "SSID"
However, we should not clear that value again on the next update:
<trace> [1603798856.5724] sup-iface[66c1a0883a262394,0,wlp2s0]: BSS /fi/w1/wpa_supplicant1/Interfaces/0/BSSs/3 updated
<debug> [1603798856.5726] device[6b383dca267b6878] (wlp2s0): wifi-ap: updated AA:BB:CC:DD:EE:FF (none)
Once we have a SSID, we can only update it to a better value,
but not clear it.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/438
Fixes: b83f07916a ('supplicant: large rework of wpa_supplicant handling')
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.
