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.
Certain properties need to release memory when destroying the NMSetting.
For "direct" properties, we have all the information we need to do that
generically in the NMSetting base class. In practice, this only concerns
string properties.
See _finalize_direct() in "nm-setting.c".
However, if the NMSetting base class takes care of freeing the strings,
then the subclasses must not also unref the variable (to avoid double free).
Previously, subclasses had to opt-in for the base class to indicate that
they are fine with that.
Now, let the base class always handle it. We only need to make sure that
classes that implement direct string properties don't also try to free
the values during destruction.
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.
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.
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.
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)
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.
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.
Most of our NMSetting properties are based around GObject properties,
and thus the tooling to convert a NMSetting to/from GVariant consists
of getting/setting a GValue.
We can do better.
For most of such properties we also define a C getter function, which
we can call with less overhead. All we need is to hook the C getter with
the property meta data.
As example, implement it for "connection.autoconnect".
The immediate goal of this is to reduce the overhead of to_dbus. But
note that also for comparison of two properties, there is the default
implementation which is used by the majority of properties. This
implementation converts the properties first to GVariant (via
to_dbus_fcn) and then compares the variants. What this commit also does,
is to hook up the property meta data with the C-getters. This is one step
towards also more efficiently compare properties using the naive C
getters. Likewise, the keyfile writer use g_object_get_property().
It also could do better.
If a property can be converted to D-Bus, then always set the
to_dbus_fcn() handler. The only caller of to_dbus_fcn() is
property_to_dbus(), so this means that property_to_dbus()
has no more default implementation and always delegates to
to_dbus_fcn().
The code is easier to understand if all properties implement
to_dbus_fcn() the same way.
Also, there is supposed to be a split between NMSettInfoProperty (info about
the property) and NMSettInfoPropertType (the type). The idea is that
each property (obviously) requires its distinct NMSettInfoProperty, but
they can share a common type implementation.
With NMSettInfoPropertType.gprop_to_dbus_fcn that is often violated because
many properties that implement NMSettInfoPropertType.gprop_to_dbus_fcn
require a special type implementation. As such, gprop_to_dbus_fcn should
be part of the property info and not the property type. The first step towards
that is unifying all properties to use to_dbus_fcn().
When subclassing a GObject type, the class and object structs
must be available and defined in the header.
For libnm, and in particular for NMSetting classes, we don't want
users to subclass NMSetting. It also doesn't work, because libnm
has internal code that is necessary to hook up the NMSetting class.
You cannot define your own type and make it work together with
libnm.
Having the structs in public headers limits what we can do with them.
For example, we could embed the private data directly in the structures
and avoid the additional indirection.
This is an API break, but for something that most likely nobody cares
about. Or better, nobody should care about. API is not what is
accidentally defined in a header, API was the library provides to
meaningfully use. Subclassing these types is not meaningful and was
only accidentally possible so far.
Only hide the structs for now. More cleanup is possible later. We shall
however aim to keep the padding and struct layout to not also break ABI.
(cherry picked from commit e46d484fae)
So far, we didn't verify the secondary connections at all.
But these really are supposed to be UUIDs.
As we now also normalize "connection.uuid" to be in a strict
format, the user might have profiles with non-normalized UUIDs.
In that case, the "connection.uuid" would be normalized, but
"connection.secondaries" no longer matches. We can fix that by
also normalizing "connection.secondaries". OK, this is not a very good
reason, because it's unlikely to affect any users in practice ('though
it's easy to reproduce).
A better reason is that the secondary setting really should be well
defined and verified. As we didn't do that so far, we cannot simply
outright reject invalid settings. What this patch does instead, is
silently changing the profile to only contain valid settings.
That has it's own problems, like that the user setting an invalid
value does not get an error nor the desired(?) outcome.
But of all the bad choices, normalizing seems the most sensible
one.
Note that in practice, most client applications don't rely on setting
arbitrary (invalid) "UUIDs". They simply expect to be able to set valid
UUIDs, which they still are. For example, nm-connection-editor presents
a drop down list of VPN profile, and nmcli also resolves connection IDs
to the UUID. That is, clients already have an intimate understanding of
this setting, and don't blindly set arbitrary values. Hence, this
normalization is unlikely to hit users in practice. But what it gives
is the guarantee that a verified connection only contains valid UUIDs.
Now all UUIDs will be normalized, invalid entries removed, and the list
made unique.
GSList requires an additional allocation for the container struct for each
element. Also, it does not have O(1) direct access. It's a pretty bad
data structure, especially if the underlying data is in form of a strv
array.
Use a GArray instead and the nm_strvarray_*() helpers.
For example for NM_SETTING_CONNECTION_SECONDARIES, the user can set
the GObject property to a string list that includes empty strings.
The C accessors (add/remove-by-value) should also accept any strings that
are accepted otherwise. Asserting against empty strings is wrong. If the
setting wants to reject empty strings, then it should use verify().
For NetworkManager profiles, "connection.uuid" is the identifier of the
profile. It is supposed to be a UUID, however:
- the UUID was not ensured to be all-lower case. We should make sure
that our UUIDs are in a consistent manner, so that users can rely
on the format of the string.
- the UUID was never actually interpreted as a UUID. It only was some
opaque string, that we use as identifier. We had nm_utils_is_uuid()
which checks that the format is valid, however that did not fully
validate the format, like it would accept "----7daf444dd78741a59e1ef1b3c8b1c0e8"
and "549fac10a25f4bcc912d1ae688c2b4987daf444d" (40 hex characters).
Both invalid UUIDs and non-normalized UUID should be normalized. We
don't want to break existing profiles that use such UUIDs, thus we don't
outright reject them. Let's instead mangle them during
nm_connection_normalize().
"libnm-core/" is rather complicated. It provides a static library that
is linked into libnm.so and NetworkManager. It also contains public
headers (like "nm-setting.h") which are part of public libnm API.
Then we have helper libraries ("libnm-core/nm-libnm-core-*/") which
only rely on public API of libnm-core, but are themself static
libraries that can be used by anybody who uses libnm-core. And
"libnm-core/nm-libnm-core-intern" is used by libnm-core itself.
Move "libnm-core/" to "src/". But also split it in different
directories so that they have a clearer purpose.
The goal is to have a flat directory hierarchy. The "src/libnm-core*/"
directories correspond to the different modules (static libraries and set
of headers that we have). We have different kinds of such modules because
of how we combine various code together. The directory layout now reflects
this.
2021-02-18 19:46:51 +01:00
Renamed from libnm-core/nm-setting-connection.c (Browse further)
