This property indicates that a non-null strv array is expected, and
an empty strv array should be returned instead of NULL if it hadn't
been created yet.
Most string properties should not accept empty strings. Add a generic
way to reject them during verify.
Add a new flag NMSettInfoProperty.direct_string_allow_empty.
Note that properties must opt-in to allow empty values. Since all
existing properties didn't have this check (but hopefully re-implemented
it in verify()), all existing properties get this flag set to TRUE.
The main point here it that new properties get the strict check by
default.
We should also review existing uses of direct_string_allow_empty,
whether the flag can be cleared. This can be done if verify() already
enforces a non-empty string, or if we accept to break behavior by
tightening up the check.
Current verifications happens by implementing NMSetting's verify().
Add code for a second step of validation, that can operate based on the
known type.
The use case will be to reject empty strings.
To embrace inclusive language, deprecate the NMSettingConnection
slave-type property and introduce port-type property.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
nm_property_compare() makes a misguided attempt to compare dictionaries
regardless of their order.
However, if variants contain duplicate keys, then the implementation
is wrong and cannot handle it correctly.
Regardless of that. While in some sense the order of dictionary keys is
irrelevant, this is not the right place to perform such normalization.
If the order of things doesn't matter, then NMSetting must normalize the
property (e.g. by sorting the keys). At that point, the GVariant shall
be compared fully.
Now that we require glib 2.42, we can use G_PARAM_EXPLICIT_NOTIFY flag.
The benefit is that this flag saves a notification, when the property
value does not change.
The downside is, that implementations of set_property() must remember to
emit _notify() when required. This is somewhat alleviated by using
_nm_setting_property_set_property_direct(), which does this
automatically.
Se the flag for G_PARAM_EXPLICIT_NOTIFY for direct boolean properties.
For now, only do it for boolean properties, because of the danger of
getting this wrong. We must review all callers to make sure that they
don't implement set_properties() and don't forget to notify.
We will deprecate "connection.master" for "connection.controller". The
old property will become an alias for the new one. That means, when
setting the property we must emit notifications for both the old and the
new property.
Add "NMSettInfoProperty.direct_also_notify" for that. This is not fully
flexible, as it only works for direct properties (duh) and only allows
to specify one addtitional GParamSpec (of the same NMSetting). It is
however sufficient for our use.
direct_hook is a union, which currently only has one member (the set function
for a direct string). Extending this might make sense, for other set functions
(e.g. overwriting setting a strv array).
However, then the name was bad. The union is already for the set-function of
direct properties, it's not a place for various kinds of hooks (as it is
a union).
Rename.
For most strv or string properties, we cannot distinguish between
NULL/unset/default and empty.
It would be difficult to enter in nmcli or grasp how it differs. There
are probably many bugs, where we accept empty strings, and fail to
handle them correctly.
Anyway. For most strv arrays, and empty array and NULL/unset/default are
treated the same. That means, g_object_get() tends to always return NULL
(never an empty strv array) and g_object_set() of an empty strv array
will internally leave the GArray at NULL.
For a few properties, there is a difference. See "ipv[46].dns-options".
See also "clear_emptyunset_fcn" hook in libnm-setting.
Add a way to handle such strv properties with the "direct" mechanism.
Unfortunately, there are several possibilities how to handle NULL and
empty arrays. Therefore we have different variants.
Clean this up, and add a way to preserve whether the array is empty
(previous variants could not distinguish that).
Functions are also renamed, so that if you backport a user of the new
API, you'll get a compiler error if this patch is missing.
Also, nm_strvarray_get_strv_notnull() no longer takes a pointer to a
"GArray*". Previously, it used that to fake an empty strv array. Now
this returns NM_STRV_EMPTY_CC().
CC src/libnm-core-impl/libnm_core_impl_la-nm-setting.lo
src/libnm-core-impl/nm-setting.c: In function '_nm_setting_class_commit':
src/libnm-core-impl/nm-setting.c:339:41: error: unused variable 'i' [-Werror=unused-variable]
339 | guint i;
| ^
src/libnm-core-impl/nm-setting.c: At top level:
src/libnm-core-impl/nm-setting.c:168:1: error: '_nm_sett_info_property_find_in_array' defined but not used [-Werror=unused-function]
168 | _nm_sett_info_property_find_in_array(const NMSettInfoProperty *properties,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Fixes: 2dd5b065a8 ('libnm: drop special casing G_TYPE_STRV from _nm_setting_class_commit()')
What we sort is very static, the names of properties in
NMSettingConnection. We know that the list contains no two identical
names. There were already assertions for that, just rework them a bit to
make the code clearer.
Previously, _nm_setting_class_commit() required that the "name" of a
NMSettInfoProperty is unset, when the property also has a "param_spec".
_nm_setting_class_commit() would then as first iterate over all
properties, and set the name.
In practice, all callers only initialize NMSettInfoProperty via
_nm_properties_override_gobj(). Now, let _nm_properties_override_gobj()
set the "name" right away.
Now _nm_setting_class_commit() will instead assert that the name is always
set, and that the caller takes care of that. That means, we have less to
do in _nm_setting_class_commit() (assertions aside).
There are no such properties left. They now all use
_nm_setting_property_define_gprop_strv_oldstyle() and the
properties_override array already contains the properties.
This simplifies _nm_setting_class_commit() and moves logic away.
Note that most of the code in _nm_setting_class_commit() is only asserts
for consistency.
Since "properties_override" now contains all properties, it doesn't
really "override" any default and the name is bad. Anyway.
The goal is to have the properties_overrides already pre-populated with
all properties. Then we will be able to drop special cases from
_nm_setting_class_commit().
Historically, the NMSetting types were in public headers. Theoretically,
that allowed users to subtype our classes. However in practice that was
impossible because they lacked required access to internal functions to
fully create an NMSetting class outside of libnm. And it also was not
useful, because you simply cannot extend libnm by subtyping a libnm
class. And supporting such a use case would be hard and limit what we can
do in libnm.
Having GObject structs in public headers also require that we don't
change it's layout. The ABI of those structs must not change, if anybody
out there was actually subclassing our GObjects.
In libnm 1.34 (commit e46d484fae ('libnm: hide NMSetting types from
public headers')) we moved the structs from headers to internal.
This would have caused a compiler error if anybody was using those
struct definitions. However, we still didn't change the ABI/layout so
that we didn't break users who relied on it (for whatever reason).
It doesn't seem there were any affected user. We waited long enough.
Change internal ABI.
No longer use g_type_class_add_private(). Instead, embed the private
structs directly (_NM_GET_PRIVATE()) or indirectly
(_NM_GET_PRIVATE_PTR()) in the object.
The main benefit is for debugging in the debugger, where we can now
easily find the private data. Previously that was so cumbersome to be
effectively impossible.
It's also the fastest possible way, since NM_SETTING_*_GET_PRIVATE()
literally resolves to "&self->_priv" (plus static asserts and
nm_assert() for type checking).
_NM_GET_PRIVATE() also propagates constness and requires that the
argument is a compatible pointer type (at compile time).
Note that g_type_class_add_private() is also deprecated in glib 2.58 and
replaced by G_ADD_PRIVATE(). For one, we still don't rely on 2.58. Also,
G_ADD_PRIVATE() is a worse solution as it supports a usecase that we
don't care for (public structs in headers). _NM_GET_PRIVATE() is still
faster, works with older glib and most importantly: is better for
debugging as you can find the private data from an object pointer.
For NMSettingIPConfig this is rather awkward, because all direct
properties require a common "klass->private_offset". This was however
the case before this change. Nothing new here. And if you ever touch
this and do something wrong, many unit tests will fail. It's almost
impossible to get wrong, albeit it can be confusing to understand.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1773
Refactor and cleanup the functions to get a setting from a connection.
As the NMConnection tracks the settings in an array indexed by
NMMetaSettingType, the most direct and efficient way is to look up via
that enum.
Previously, nm_connection_get_setting_by_name() would first look up the GType
(which already involved looking up the NMMetaSettingInfo), then based on the
GType it would look up the NMMetaSettingInfo again to get the meta_type. That
is unnecessary. Directly look up the NMMetaSettingInfo, which directly
gives the meta_type.
Like for our other immutable/sealable types, make ref/unref thread safe.
That is important, as the boxed types only increase the ref-count on
copy. If ref/unref is not thread-safe, it means you cannot copy a boxed
type, and operate on the copy on another thread.
Fixes: 041e38b151 ('libnm: add NMRange')
GArray.data is a char pointer. Most of the time we track other data in
a GArray. Casting that pointer can trigger "-Wcast-align=strict"
warnings.
Avoid them. Most of the time, instead use the nm_g_array*() helpers,
which also assert that the expected element size is correct.
The next commit is going to introduce a new object in libnm to
represent a range of ovs-port VLANs. A "range of integers" object
seems something that can be used for other purposes in the future, so
instead of adding an object specific for this case
(e.g. NMOvsPortVlanRange), introduce a generic NMRange object that
generically represents a range of non-negative integers.
- the static assertions were wrong, there was a "," instead of "==".
- the numeric values were wrong, as shown by the static assertions.
- move the code comment to the implementation. This does not seem
relevant for the library user and should not be in the public header.
Fixes: 08e845f651 ('nm-setting: mangle public constant to make g-ir-scanner happy')
property_to_dbus() gets called for two reasons. Once from
_nm_setting_to_dbus(). In that case, we want to honor
to_dbus_only_in_manager_process().
It gets also called from _nm_setting_property_compare_fcn_default(),
with ignore_flags set. In that case, we don't want to ignore the property
as the hook really wants to compare them.
Fixes: c8392018ca ('libnm: refactor to-dbus on the client skipping to serialize legacy properties')
The previous could would first check whether the new property is not
set. In almost all cases, the new property is actually set.
We can get away with fewer lookups, by checking for the expected things
first.
We have 4 legacy properties ("ipv[46].addresses", "ipv[46].routes") that
got replaced by newer variants ("ipv[46].address-data", "ipv[46].route-data").
When the client side of libnm (_nm_utils_is_manager_process) serializes
those properties, it must only serialize the newer version. That is so
that the forward/backward compatibility works as intended.
Previously, there was the NM_SETTING_PARAM_LEGACY GObject property flag.
That was fine, but not very clear.
For one, the legacy part of those properties is only about D-Bus. In
particular, they are not deprecated in libnm, keyfile, or nmcli. Thus
the name wasn't very clear.
Also, in the meantime we have more elaborate property meta data, that
goes beyond the meta data of the GObject property.
Move NM_SETTING_PARAM_LEGACY to NMSettInfoProperty.to_dbus_only_in_manager_process.
I think, this is a better name. It's also right at
```
_nm_properties_override_gobj(
properties_override,
g_object_class_find_property(G_OBJECT_CLASS(setting_class), NM_SETTING_IP_CONFIG_ROUTES),
NM_SETT_INFO_PROPERT_TYPE_DBUS(NM_G_VARIANT_TYPE("a(ayuayu)"),
.to_dbus_fcn = ip6_routes_to_dbus,
.compare_fcn = _nm_setting_ip_config_compare_fcn_routes,
.from_dbus_fcn = ip6_routes_from_dbus, ),
.to_dbus_only_in_manager_process = TRUE,
.dbus_deprecated = TRUE, );
```
that is, directly at the place where we describe how the D-Bus property behaves.
When an authentication attempt fails, NetworkManager re-requests new secrets
from agents before retrying. This is currently decided outside of the NMSetting
objects. With this change the decision if a re-request of new secrets is really
needed is moved down to the NMSetting implementations.
For the case "802.1x authentication with TLS" a certificate with password is
configured and the assumption is, that this can never be wrong and no re-request
is needed.
Have "len" before "elem_size". That is consistent with g_qsort_with_data()
and bsearch(), and is also what I would expect.
Note that the previous commit just renamed the function. If a user
of the new, changed API gets backported to an older branch, we will
get a compilation error and note that the arguments need to be adjusted.
The "nm_utils_" prefix is just too verbose. Drop it.
Also, Posix has a bsearch function. As this function
is similar, rename it.
Note that currently the arguments are provided in differnt
order from bsearch(). That will be partly addressed next.
That is the main reason for the rename. The next commit
will swap the arguments, so do a rename first to get a compilation
error when backporting a patch that uses the changed API.
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
This is severe. We cache the list of names, and we must invalidate the
cache when the names change. Otherwise, out-of-bound access and crash.
Fixes: d0192b698e ('libnm: add nm_setting_option_set(), nm_setting_option_get_boolean(), nm_setting_option_set_boolean()')
Fixes: 150af44e10 ('libnm: add nm_setting_option_get_uint32(), nm_setting_option_set_uint32()')
