Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability.
We should generate the GVariant in a stable manner. That implies
to sort the keys first.
Also, don't use the NM_SETTING_VPN_SECRETS getter, which first needs
to clone all secrets.
When we use _nm_utils_strdict_from_dbus(), we clone the secrets, but don't use
nm_free_secret() for freeing them.
And in fact, we clone the setings twice. It't really not too nice. Implement
this without the property setter.
Don't use _nm_utils_copy_strdict().
On a minor note, that function will always allocate a GHashTable, even if
it only contains "" as only key. Later we would throw that out again,
so it was unnecessary.
Worse, using _nm_utils_copy_strdict() does not use nm_free_secrets as
destroy function. While it's in general difficult to clear all places
in memory where we copy the secrets around, we can easily avoid that.
Also skip over %NULL keys and values. It probably would be a bug passing
such arguments to the property. Better ignore them and not crash.
Like for data, now also allow empty secrets to be added to the VPN
setting.
For one, this avoids an assertion failure, where keyfile reader wouldn't
check whether a secret key is set to the empty word.
For data, it's more clear that we want to allow setting empty data
values. VPN settings are only interpreted by the VPN plugin, so libnm
and the daemon shouldn't prevent empty settings. It can be useful to
distinguish between unset (NULL) and empty values.
For secrets, it's less clear that empty secrets shall be allowed. I
think it should. Of course, the empty secret likely isn't a correct
nor valid secret. But libnm cannot validate the secrets anyway. It's
up to the VPN plugin to handle this in any way they see fit.
Also, already before, the user could set NM_SETTING_VPN_SECRETS to
a string dictionary with empty passwords. So, the API didn't fully
prevent that. Only certain API wouldn't play along.
Until now, nm_setting_vpn_add_data_item() would reject empty data values.
This leads for example to an assertion failure, if you write a keyfile
that assigns an empty value to a key. Keyfile reader would not check that
the value is non-empty before calling nm_setting_vpn_add_data_item().
Anyway, I think we should not require having non-empty data elements. It's
an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand
not care about the content of the vpn data. That is up the VPN plugins. Sometimes
and empty value may be desirable.
Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty
values either. So it was always possible to use some libnm API to set data
with empty values. The restriction in nm_setting_vpn_add_data_item() was
inconsistent.
Also drop the g_warn_if_fail() from update_secret_dict(). We
may get the variant from D-Bus, so avoiding this assertion (g_warn*() is
an assertion!) would require us to prevalidate the variant.
That would be very cumbersome, and we would probably not want to
handle that as an error and silently ignore them anyway. Just shut
up.
Ensure that the data hash doesn't contain keys with empty key-name.
It just doesn't make sense, and will lead to potential problems later,
if we would allow this to happen.
For example, keyfile writer may naively try to set all keys, without
checking for empty keys. That may lead to assertion failures or worse,
later on. Don't allow that.
Also, assert against non-empty keys in nm_setting_vpn_get_data_item()
and nm_setting_vpn_remove_data_item(). This stricter handling is a
change in behavior, however it would have always been a bug trying to
refer to empty key names. So, this assertion will only help to find
those bugs.
We always initialized priv->data in nm_setting_vpn_init(), but usually
soon after this hash would be replaced by NM_SETTING_VPN_DATA property
setter.
Avoid that. Allow for priv->data to be %NULL, which of course has the
meaning that no keys are set.
When we invoke the user's callback, be more robust about the user
modifying or unreferencing the NMSettingVpn, while iterating.
While it would be odd to modify the NMSettingVpn from inside the foreach
callback, we should behave consistently and sensibly.
That means, to ensure that the NMSettingVpn instance stays alive all
the time, that we don't crash, and that we always iterate over the
previously determined, planned list of keys.
Also, avoid some unnecessary string copies, like the clone of the first key.
For one, this code was unreachable, because we checked these conditions
already before.
That aside, g_warn_*() is for all intended purposes an assertion.
The caller probably gets the GVariant from an untrusted source (e.g. via
D-Bus). Asserting here is not helpful.
It's up to the caller to validate the argument. Or, in case the caller
doesn't care, update_secret_dict() should just do the sensible thing.
But be silent about it!
Yes, the compiler probably can optimize strlen() in these cases. That
still doesn't make strlen() the right API to check whether a NUL
terminated string is empty.
Sometimes these function may set errno to unexpected values like EAGAIN.
This causes confusion. Avoid that by using our own wrappers that retry
in that case. For example, in rhbz#1797915 we have failures like:
errno = 0;
v = g_ascii_strtoll ("10", 0, &end);
if (errno != 0)
g_assert_not_reached ();
as g_ascii_strtoll() would return 10, but also set errno to EAGAIN.
Work around that by using wrapper functions that retry. This certainly
should be fixed in glib (or glibc), but the issues are severe enough to
warrant a workaround.
Note that our workarounds are very defensive. We only retry 2 times, if
we get an unexpected errno value. This is in the hope to recover from
a spurious EAGAIN. It won't recover from other errors.
https://bugzilla.redhat.com/show_bug.cgi?id=1797915
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
This solves a bug exposed by the following cmds:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr
Here we just added the option 'miimon=100', but it doesn't get saved in
because nm_settings_connection_set_connection() which is responsible for
actually updating the connection compares the new connection with old
one and if and only if the 2 are different the update is carried out.
The bug is triggered because when comparing, if default values are taken into
account, then having 'miimon=100' or not having it it's essentially the
same for compare(). While this doesn't cause a bond to have a wrong
setting when activated it's wrong from a user experience point of view
and thus must be fixed.
When this patch is applied, the above
commands will give the following results:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr,miimon=100
Fix unit tests and also add a new case covering this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=1806549
Of course, having no list does not mean we cannot resolve the service-type.
That is, because we also have a hard-coded list of known VPNs.
Fixes: 67c00353d3 ('libnm: reuse _list_find_by_service() for searching NMVpnPluginInfo')
The NMVpnPluginInfo class is not intended to be subclassed. An API that
allows to be subclassed needs to be designed in a certain manner for
that to be useful. NMVpnPluginInfo does not want to support that.
Only because a user technically could do that (as the structs were
in the public headers), it does not make it supported. Not everything
that is possible in C is guaranteed to work.
Also, of course there exist no users in practice that would rely on this.
So, hide the structs.
Also, this allows to embed the private data in the GObject struct
itself, which is useful for debugging and for performance.
list-load does some special handling, for example, it will avoid adding
duplicates. As such, two plugin infos cannot have the same name or
same service type.
nm_vpn_plugin_info_new_search_file() did not implement this, it merely
loaded each directory after the other, sort the plugin infos, and
returned the first match.
That might mean, with unusual (duplicate) name files,
nm_vpn_plugin_info_new_search_file() might return a value
that would not otherwise be returned by
nm_vpn_plugin_info_list_load().
Let nm_vpn_plugin_info_new_search_file() call list-load, so that
the search result is always consistent.
The downside of this is that previously, if the searched plugin was
already found in /usr/lib, we would skip loading /etc. But
that is a minor optimization, in any case nm_vpn_plugin_info_new_search_file()
scales with the number of .name files on disk, which is expected to be small.
Add 'nm_setting_bond_get_option_normalized()', the purpose of this API
is to retrieve a bond option normalized value which is the option that
NetworkManager will actually apply to the bond when activating the
connection, this takes into account default values for some options that
NM assumes.
For example, if you create a connection:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0
Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return "100" as even if not specified NetworkManager enables miimon for
bond connections.
Another example:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0,arp_interval=100
Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return NULL in this case because NetworkManager disables miimon if
'arp_interval' is set explicitly but 'miimon' is not.
Add '_nm_setting_bond_get_option_or_default()' and move all the custom
policies applied by NM for bond options in there.
One such example of a custom policy is to set 'miimon' to 0 (instead of its
default value of 100) if 'arp_interval' is explicitly enabled
and 'miimon' is not.
This means removing every piece of logic from
nm_setting_bond_add_option() which used to clear out 'arp_interval' and
'arp_ip_target' if 'miimon' was set or clear out 'miimon' along with
'downdelay', 'updelay' and 'miimon' if 'arp_interval' was set.
This behaviour is a bug since the kernel allow setting any combination
of this options for bonds and NetworkManager should not limit the user
to do so.
Also use 'set_bond_attr_or_default()' instead of 'set_bond_attr()' as
the former calls '_nm_setting_bond_get_option_or_default()' to implement
the right logic to retrieve bond options according to current bond
configuration.
Fix 'miimon' and 'arp_interval' validation, they can both be set indeed,
the kernel does not impose this limitation, nevertheless is sensible to
keep the defaults as previously (miimon=100, arp_interval=0).
Also add unit test.
Doing 'verify()' with options such as 'miimon' and 'num_grat_arp' set to
arbitrary values it's not consistent with what NetworkManager does to
bond options when activating the bond through 'apply_bonding_config()'
(at a later stage) because the said values do not
correspond to what the default values for those options are.
This leads to an inconsistency with the 'miimon' parameter for example,
where 'verify()' is done while assuming it's 0 if not set but its
default value is actually 100.
Fixes: 8775c25c33 ('libnm: verify bond option in defined order')
