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.
"flags" are a g_param_spec_flags() and correspond to G_TYPE_FLAGS type.
They are internally stored as guint, and exported on D-Bus as "u" (32 bit
integer).
String properties in libnm's NMSetting really should have NULL as a
default value. The only property that didn't, was "dcb.app-fcoe-mode".
Change the default so that it is also NULL.
Changing a default value is an API change, but in this case probably no
issue. For one, DCB is little used. But also, it's not clear who would
care and notice the change. Also, because previously verify() would reject
a NULL value as invalid. That means, there are no existing, valid profiles
that have this value set to NULL. We just make NULL the default, and
define that it means the same as "fabric".
Note that when we convert integer properties to D-Bus/GVariant, we often
omit the default value. For string properties, they are serialized as
"s" variant type. As such, NULL cannot be expressed as "s" type, so we
represent NULL by omitting the property. That makes especially sense if
the default value is also NULL. Otherwise, it's rather odd. We change
that, and we will now always express non-NULL value on D-Bus and let
NULL be encoded by omitting the property.
Give a consistent name.
A bit odd are now the names nm_g_bytes_hash() and nm_g_bytes_equal()
as they go together with nm_pg_bytes_hash()/nm_pg_bytes_equal().
But here the problem is more with the naming of "nm_p*_{equal,hash}()"
functions, which probably should be renamed to "nm_*_ptr_{equal,hash}()".
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.
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.
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.
On architectures where "char" is signed, the check "ch < ' '" is
also TRUE for non-ASCII characters greater than 127. This is an
easy mistake to make. Fix it by using nm_ascii_is_control() which
gets this right.
It's a bug, but possibly not too bad because unnecesarily escaping
a UTF-8 characters is not a severe problem, because the user anyway must
be prepared to unescape the string.
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.
NM_SETTING_NAME is also a GObject property, but it's
not supposed to be serialized to/from D-Bus. It also
is irrelevant for comparison.
Hence, it's operations are all NOPs. Make an explicit property type for
that case instead of checking the GParamSpec flags.
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.
All settings have a "name" property. Their compare_fcn() is not interesting
and was already previously ignored. But we should not special handle it via
_nm_setting_property_compare_fcn_default().
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.
We encode the default value "direct" properties in the GParamSpec.
But we also avoid CONSTRUCT properties, because they have an overhead
and they are generally odd for the settings.
So up to now, it was cumbersome to explicitly set the default value,
but it was also error prone.
Avoid that by always initializing the default value for our "direct"
properties.
And as example, implement NMSettingVrf.table this way. This also
makes all properties of NMSettingVrf implemened as "direct" properties,
and we can drop the explicit getter/setters.
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.
NMSetting instances either have no private data, they use
g_type_add_class_private(), or they embed the private data in the
NMSetting struct.
In all cases, we can find the private data at a fixed offset. Track that
offset in the NMSettInfoSetting meta data.
This will be useful, because properties really are stored in simple
fields, like a boolean property can be stored in a "bool" field. We will
extend the property meta data to track the offset of this property
field, but we also need to know where the offset starts.
"ipv6.method=ignore" really exists for historic reasons, from a time when
NetworkManager didn't support IPv6 autoconf and let kernel handle it.
Nowadays, we should choose an explicit mode, like "link-local" or
"disabled".
Let nm_connection_normalize() treat WireGuard and dummy profiles
different and set the IPv6 method to "disabled".
Add a new property to specify the minimum time interval in
milliseconds for which dynamic IP configuration should be tried before
the connection succeeds.
This property is useful for example if both IPv4 and IPv6 are enabled
and are allowed to fail. Normally the connection succeeds as soon as
one of the two address families completes; by setting a required
timeout for e.g. IPv4, one can ensure that even if IP6 succeeds
earlier than IPv4, NetworkManager waits some time for IPv4 before the
connection becomes active.
