Add a new "ovs-port.trunks" property that indicates which VLANs are
trunked by the port.
At ovsdb level the property is just an array of integers; on the
command line, ovs-vsctl accepts ranges and expands them.
In NetworkManager the ovs-port setting stores the trunks directly as a
list of ranges.
Support managing the loopback interface through NM as the users want to
set the proper mtu for loopback interface when forwarding the packets.
Additionally, the IP addresses, DNS, route and routing rules are also
allowed to configure for the loopback connection profiles.
https://bugzilla.redhat.com/show_bug.cgi?id=2060905
These are just general purpose D-Bus utils, based on glib and GDBus.
They fit perfectly to libnm-glib-aux. Move the code.
Also, there is already the file "src/core/nm-dbus-utils.c", having two
files with the same name on our source tree is just confusing.
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.
NMConnection is an interface, implemented by NMSimpleConnection and
NMRemoteConnection.
For the most part, an NMConnection is only the content of the profile
(the settings). The "path" of the connection refers to the D-Bus path,
and wouldn't really make sense of the NMConnection interface or the
NMSimpleConnection type.
As such, the daemon (which only uses NMConnection and
NMSimpleConnection) never sets the path. Only libnm does.
NMClient uses NMRefString extensively for the D-Bus interface and the
path is already internalized. Take advantage of that. It is very likely,
that we are able to share the path instance in libnm at which point it
makes sense to use NMRefString.
Also, during nm_simple_connection_new_clone(), we can just take another
reference instead of cloning the string.
Bond option values are just strings, however, some of them get
validated to be numbers, etc.
We also have effectively boolean values, like "use-carrier". Internally,
this is not validates as a boolean (_nm_utils_ascii_str_to_bool()) but
instead is an integer of either "0" or "1".
Add a helper function_nm_setting_bond_opt_value_as_intbool() to access
and parse such values.
The bond setting does some minimal validation of the options.
At least for those number typed values, it validates that the
string can be interpreted as a number and is within a certain range.
Add nm_assert() checks to our opt_value_u$SIZE() functions, that the
requested option is validated to be in a range which is sufficiently
narrow to be converted to the requested type. If that were not the case,
we would need some special handling (or question whether the option should
be retrieved as this type).
Currently nm_setting_bond_get_option_normalized() and
nm_setting_bond_get_option_or_default() are identical functions. As the
first one is exposed as public API and has a better name, let's drop the
second one.
All callers of _nm_setting_get_private() got the offset from the
property info. Add a wrapper _nm_setting_get_private_field() that
takes the property info. This way, it can add some assertions.
Several properties like "connection.type" are enum-like and only take a few
known values. We can use a NMRefString to share their instances.
Currently nm_setting_duplicate() does not yet explicitly handle direct properties.
But it should, because it can handle them more efficiently. If it would do that, it
would be very cheap to "copy" a NMRefString. But even with the current implementation
will the result be deduplicated.
We want that our properties have little special cases and follow a
few common behaviors. For example, we have string properties, and those
should mostly behave the same (e.g. by being "direct-string"
properties).
That is already not fully enough, because we have slightly different
behaviors. For example, we have string properties that should have their
whitespace stripped, that should be ascii case down converted, that
should be normalized IP or MAC addresses. So far, that was expressed via
simple fields in NMSettInfoProperty, like NMSettInfoProperty's
direct_set_string_ascii_strdown field.
But that is not enough. In particular, for "wireguard.private-key" we
perform a different kind of normalization (base64 parsing, and taking
care not to leak secret in memory). It seems to special to add a boolean
flag "direct_set_string_wireguard_private_key".
Instead, add a hook that can cover that.
We need a hook, because we want one setter implementation throughout. Commonly,
we have at least two setters: the GObject set_property() and from D-Bus.
Both should call into the same underlying implementation, to avoid code
duplication. For that, the tweaked behavior must be "down", that is at
the deepest point in the call stack where we set the string. That's why
we need the hook. The alternative would be two special implementation
for GObject and D-Bus setters (and in the future we might add setters
from keyfile).
This seems a questionable thing to do, and should be made clearer by
having a parameter (that makes you think about what is happening here).
Also, the normalization for vxlan.remote does not perform this mapping,
so the parameter is there so that the approach can handle both flavors.
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
"direct" properties are the latest preferred way to implement GObject
base properties. That way, the property meta data tracks the
"direct_type" and the offset where to find the data in the struct.
That way, we can automatically
- initialize the default values
- free during finalize
- implement get_property()/set_property()
Also, the other settings operations (compare, to/from D-Bus) are
implemented more efficiently and don't need to go through
g_object_get_property()/GValue API.
The name prefix "nmtst_*" is reserved for test helpers and stub
function. Such functions should not be in the actual build artifacts,
like the NetworkManager binary.
Instead, nmtst_connection_assert_unchanging() is not a test helper. It
is a assertion function that is only enabled with NM_MORE_ASSERTS
builds. That's different.
Rename.
In other words,
$ nm src/core/NetworkManager src/libnm-client-impl/.libs/libnm.so | grep nmtst
should give no results.
These type-specific getters are not very useful. _nm_connection_get_setting() is
better because the setting type is a parameter so they can be used more generically.
Have less code and use generic helpers.
Note that most implementations use g_object_set(), and it's not
easy to detect modification. In those cases, we assume that modification
happened -- just like also the GObject setter will emit a notification
(as none of our properties use G_PARAM_EXPLICIT_NOTIFY).
These functions tend to have many arguments. They are also quite som
boilerplate to implement the hundereds of properties we have, while
we want that properties have common behaviors and similarities.
Instead of repeatedly spelling out the function arguments, use a macro.
Advantages:
- the usage of a _NM_SETT_INFO_PROP_*_FCN_ARGS macro signals that this
is an implementation of a property. You can now grep for these macros
to find all implementation. That was previously rather imprecise, you
could only `git grep '\.to_dbus_fcn'` to find the uses, but not the
implementations.
As the goal is to keep properties "similar", there is a desire to
reduce the number of similar implementations and to find them.
- changing the arguments now no longer will require you to go through
all implementations. At least not, if you merely add an argument that
has a reasonable default behavior and does not require explicit
handling by most implementation.
- it's convenient to be able to patch the argument list to let the
compiler help to reason about something. For example, the
"connection_dict" argument to from_dbus_fcn() is usually unused.
If you'd like to find who uses it, rename the parameter, and
review the (few) compiler errors.
- it does save 573 LOC of boilerplate with no actual logic or useful
information. I argue, that this simplifies the code and review, by
increasing the relative amount of actually meaningful code.
Disadvantages:
- the user no longer directly sees the argument list. They would need
cscope/ctags or an IDE to jump to the macro definition and conveniently
see all arguments.
Also use _nm_nil, so that clang-format interprets this as a function
parameter list. Otherwise, it formats the function differently.
Naming is important, because the name of a thing should give you a good
idea what it does. Also, to find a thing, it needs a good name in the
first place. But naming is also hard.
Historically, some strv helper API was named as nm_utils_strv_*(),
and some API had a leading underscore (as it is internal API).
This was all inconsistent. Do some renaming and try to unify things.
We get rid of the leading underscore if this is just a regular
(internal) helper. But not for example from _nm_strv_find_first(),
because that is the implementation of nm_strv_find_first().
- _nm_utils_strv_cleanup() -> nm_strv_cleanup()
- _nm_utils_strv_cleanup_const() -> nm_strv_cleanup_const()
- _nm_utils_strv_cmp_n() -> _nm_strv_cmp_n()
- _nm_utils_strv_dup() -> _nm_strv_dup()
- _nm_utils_strv_dup_packed() -> _nm_strv_dup_packed()
- _nm_utils_strv_find_first() -> _nm_strv_find_first()
- _nm_utils_strv_sort() -> _nm_strv_sort()
- _nm_utils_strv_to_ptrarray() -> nm_strv_to_ptrarray()
- _nm_utils_strv_to_slist() -> nm_strv_to_gslist()
- nm_utils_strv_cmp_n() -> nm_strv_cmp_n()
- nm_utils_strv_dup() -> nm_strv_dup()
- nm_utils_strv_dup_packed() -> nm_strv_dup_packed()
- nm_utils_strv_dup_shallow_maybe_a() -> nm_strv_dup_shallow_maybe_a()
- nm_utils_strv_equal() -> nm_strv_equal()
- nm_utils_strv_find_binary_search() -> nm_strv_find_binary_search()
- nm_utils_strv_find_first() -> nm_strv_find_first()
- nm_utils_strv_make_deep_copied() -> nm_strv_make_deep_copied()
- nm_utils_strv_make_deep_copied_n() -> nm_strv_make_deep_copied_n()
- nm_utils_strv_make_deep_copied_nonnull() -> nm_strv_make_deep_copied_nonnull()
- nm_utils_strv_sort() -> nm_strv_sort()
Note that no names are swapped and none of the new names existed
previously. That means, all the new names are really new, which
simplifies to find errors due to this larger refactoring. E.g. if
you backport a patch from after this change to an old branch, you'll
get a compiler error and notice that something is missing.
Introduce a user tag key to indicate where the connection comes
from. It would also be possible to have this as a standard property
(as 'connection.origin'), but since this information can be considered
'meta-data' I think the user setting is more appropriate.
There is a quest to move away from the GObject/GValue based setters.
Add _nm_setting_property_from_dbus_fcn_direct(), which can parse
the GVariant and use the direct_type to set the property.
Note that for backward compatibility, we still need
_nm_property_variant_to_gvalue() to convert alternative GVariant
types to the destination value. This means, as before, on the D-Bus
API a property of a certain type can be represented as various D-Bus
types.
This is a normalization employed by NMSettingIPConfig.gateway.
Also rework NMSettingIPConfig.set_property() to no longer assert against
valid input. We want to pass there untrusted strings from D-Bus,
asserting is a horrible idea. Instead, either normalize the string or
keep the invalid text that will be rejected by verify().
A MAC address is a relatively common "type". The GObject property is of type string,
but the D-Bus type is a bytestring ("ay"). We will need a special NMSettInfoPropertType.
Note that like most implementations, the from-dbus implementation still is based
on GObject setters. This will change in the future.
Also note that the previous compare function was
_nm_setting_property_compare_fcn_default(). That is, it used to convert
the property to GVariant and compare those. The conversion to GVariant
in that case normalizes the string (e.g. it is case insensitive). Also,
only properties could be compared which were also convertible to D-Bus
(which is probably fine, because there is no guarantee the profiles that
don't verify can be compared).
The code now uses the direct comparison of the strings. That mostly
preserves the case-insensitivity of the previous comparison, because
the property setters for mac addresses all use
_nm_utils_hwaddr_canonical_or_invalid() to normalize the strings.
This is subtle, but still correct. Note that this will improve later,
by ensuring that the property setters for mac addresses automatically
perform the right normalization.
When looking at a property, it should always be clear how it is handled.
Also the "default" action should be an explicit hook.
Add _nm_setting_property_from_dbus_fcn_gprop() and set that as
from_dbus_fcn() callback to handle the "default" case which us
build around g_object_set_property().
While this adds lines of code, I think it makes the code easier to
understand. Basically, to convert a GVariant to a property, now all
properties call their from_dbus_fcn() handler, there is no special casing.
And the gprop-hook is only called for properties that are using
_nm_setting_property_from_dbus_fcn_gprop(). So, you can reason about
these two functions at separate layers.
The "to_dbus_data" existed for namespacing the properties inside it.
However, such a struct adds overhead due to the alignment that it
enforces. We can share the memory needed for the bitfield by having
them beside each other.
The goal is to get rid of gprop_to_dbus_fcn() uses.
Note that there is a change in behavior. The "dns" GPtrArray in
NMSettingIPConfig is never NULL (the default of the boxed property),
thus the previous code always serialized the property, even the
empty list.
Now, empty dns properties are omitted from D-Bus.
So far, we only have NMSettingClass.compare_property() hook.
The ugliness is that this hook is per-setting, when basically
all implementations only compare one property.
It feels cleaner to have a per-property hook and call that consistently.
In step one, we give all properties (the same) compare_fcn() implementation,
which delegates to the existing NMSettingClass.compare_property().
In a second step, this will be untangled.
There is one problem with this approach: NMSettInfoPropertType grows by
one pointer size, and we have potentially many such types. That should
be addressed by unifying types in the future.
Various NMSetting API would accept a property_idx parameter. Together
with the NMSettInfoSetting instance, this was useful to find the actual
NMSettInfoProperty instance.
The idea was, to provide the most of the functionality. That is, if you
might need the property_idx too, you had it -- after all, the
property_info you could lookup yourself.
However,
- literally zero users care about the property_idx. The care about
the property_info.
- if the user really, really required the property_idx, then it
is a given that it can be easily computed by
(property_info - sett_info->property_infos)
Introduce a new mechanism for how to handle properties generically.
We have NMSettInfoSetting, NMSettInfoProperty and NMSettInfoPropertType
with meta data about settings and their properties.
For example, we have a simple boolean property. Then (usually) we have a
boolean GParamSpec, and a plain boolean field in the NMSetting's private
data. We need very little to get (and convert to keyfile, GVariant),
set (from keyfile, GVariant) and compare this property.
All we need to know, is the GParamSpec and the offset of the bool field.
Introduce a new mechanism for that, and as example implement
NM_SETTING_CONNECTION_AUTOCONNECT property this way.
Note that this patch only changes the to_dbus_fcn() for the boolean
property. But this opens up all kind of further improvements.
What we eventually also can do is replace GObjectClass.get_property()
with a generic variant, that knows how to get and set the property.
