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.
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.
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.
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.
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.
The advantage is that we use similar macros for initializing the static structs like const NMSettInfoPropertType nm_sett_info_propert_type_cloned_mac_address; and the ad-hoc locations that use NM_SETT_INFO_PROPERT_TYPE(). The former exist for property types that are used more than once. The latter exist for convenience, where a property type is implemented at only one place. Also, there are few direct references to _nm_setting_property_to_dbus_fcn_gprop(). all users use NM_SETT_INFO_PROPERT_TYPE_GPROP() or NM_SETT_INFO_PROPERT_TYPE_GPROP_INIT().
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().