We should generate the GVariant in a stable manner. That implies
to sort the keys first.
Also, don't use the NM_SETTING_VPN_SECRETS getter, which first needs
to clone all secrets.
When we use _nm_utils_strdict_from_dbus(), we clone the secrets, but don't use
nm_free_secret() for freeing them.
And in fact, we clone the setings twice. It't really not too nice. Implement
this without the property setter.
Don't use _nm_utils_copy_strdict().
On a minor note, that function will always allocate a GHashTable, even if
it only contains "" as only key. Later we would throw that out again,
so it was unnecessary.
Worse, using _nm_utils_copy_strdict() does not use nm_free_secrets as
destroy function. While it's in general difficult to clear all places
in memory where we copy the secrets around, we can easily avoid that.
Also skip over %NULL keys and values. It probably would be a bug passing
such arguments to the property. Better ignore them and not crash.
Like for data, now also allow empty secrets to be added to the VPN
setting.
For one, this avoids an assertion failure, where keyfile reader wouldn't
check whether a secret key is set to the empty word.
For data, it's more clear that we want to allow setting empty data
values. VPN settings are only interpreted by the VPN plugin, so libnm
and the daemon shouldn't prevent empty settings. It can be useful to
distinguish between unset (NULL) and empty values.
For secrets, it's less clear that empty secrets shall be allowed. I
think it should. Of course, the empty secret likely isn't a correct
nor valid secret. But libnm cannot validate the secrets anyway. It's
up to the VPN plugin to handle this in any way they see fit.
Also, already before, the user could set NM_SETTING_VPN_SECRETS to
a string dictionary with empty passwords. So, the API didn't fully
prevent that. Only certain API wouldn't play along.
Until now, nm_setting_vpn_add_data_item() would reject empty data values.
This leads for example to an assertion failure, if you write a keyfile
that assigns an empty value to a key. Keyfile reader would not check that
the value is non-empty before calling nm_setting_vpn_add_data_item().
Anyway, I think we should not require having non-empty data elements. It's
an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand
not care about the content of the vpn data. That is up the VPN plugins. Sometimes
and empty value may be desirable.
Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty
values either. So it was always possible to use some libnm API to set data
with empty values. The restriction in nm_setting_vpn_add_data_item() was
inconsistent.
Also drop the g_warn_if_fail() from update_secret_dict(). We
may get the variant from D-Bus, so avoiding this assertion (g_warn*() is
an assertion!) would require us to prevalidate the variant.
That would be very cumbersome, and we would probably not want to
handle that as an error and silently ignore them anyway. Just shut
up.
Ensure that the data hash doesn't contain keys with empty key-name.
It just doesn't make sense, and will lead to potential problems later,
if we would allow this to happen.
For example, keyfile writer may naively try to set all keys, without
checking for empty keys. That may lead to assertion failures or worse,
later on. Don't allow that.
Also, assert against non-empty keys in nm_setting_vpn_get_data_item()
and nm_setting_vpn_remove_data_item(). This stricter handling is a
change in behavior, however it would have always been a bug trying to
refer to empty key names. So, this assertion will only help to find
those bugs.
We always initialized priv->data in nm_setting_vpn_init(), but usually
soon after this hash would be replaced by NM_SETTING_VPN_DATA property
setter.
Avoid that. Allow for priv->data to be %NULL, which of course has the
meaning that no keys are set.
When we invoke the user's callback, be more robust about the user
modifying or unreferencing the NMSettingVpn, while iterating.
While it would be odd to modify the NMSettingVpn from inside the foreach
callback, we should behave consistently and sensibly.
That means, to ensure that the NMSettingVpn instance stays alive all
the time, that we don't crash, and that we always iterate over the
previously determined, planned list of keys.
Also, avoid some unnecessary string copies, like the clone of the first key.
For one, this code was unreachable, because we checked these conditions
already before.
That aside, g_warn_*() is for all intended purposes an assertion.
The caller probably gets the GVariant from an untrusted source (e.g. via
D-Bus). Asserting here is not helpful.
It's up to the caller to validate the argument. Or, in case the caller
doesn't care, update_secret_dict() should just do the sensible thing.
But be silent about it!
Yes, the compiler probably can optimize strlen() in these cases. That
still doesn't make strlen() the right API to check whether a NUL
terminated string is empty.
"nm-setting.c" (and property_to_dbus()) should stay independent of
actualy settings implementations. Instead, the property-info should
control the behavior.
What I like about this change is also that the generic handling is not a
flags "handle_secrets_for_vpn", but it just says to skip checking the
param-spec flags and directly call the to_dbus_fcn(). It's just a
generally useful thing to do, to let the to_dbus_fcn() function also
handle checking the property flags. The fact that only vpn.secrets
properties uses this for a certain pupose, is abstracted in a way that
makes sense.
In total, we register 447 property informations. Out of these,
326 are plain, GObject property based without special implementations.
The NMSettInfoProperty had all function pointers directly embedded,
currently this amounts to 5 function pointers and the "dbus_type" field.
That means, at runtime we have 326 times trivial implementations with
waste 326*6*8 bytes of NULL pointers. We can compact these by moving
them to a separate structure.
Before:
447 * 5 function pointers
447 * "dbus_type" pointer
= 2682 pointers
After:
447 * 1 pointers (for NMSettInfoProperty.property_type)
89 * 6 pointers (for the distinct NMSettInfoPropertType data)
= 981 pointers
So, in total this saves 13608 byes of runtime memory (on 64 bit arch).
The 89 NMSettInfoPropertType instances are the remaining distinct instances.
Note that every NMSettInfoProperty has a "property_type" pointer, but most of them are
shared. That is because the underlying type and the operations are the same.
Also nice is that the NMSettInfoPropertType are actually constant,
static fields and initialized very early.
This change also makes sense form a design point of view. Previously,
NMSettInfoProperty contained both per-property data (the "name") but
also the behavior. Now, the "behavioral" part is moved to a separate
structure (where it is also shared). That means, the parts that are
concerned with the type of the property (the behavior) are separate
from the actual data of the property.
property_to_dbus() returns NULL when called with
NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED and the property is
not an agent-owned secrets. The function doesn't handle VPN secrets
correctly, since they are all stored as a hash in the vpn.secrets
property and the flag for each of them is a matching '*-flags' key in
the vpn.data property. VPN secrets must be handled differently; do it
in the VPN setting to_dbus_fcn() function.
Fixes: 71928a3e5c ('settings: avoid cloning the connection to maintain agent-owned secrets')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/230https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/280
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
We have certain artificial properties that not only depend on one
property alone or that depend on a property in another(!) setting.
For that, we have synth_func.
Other than that, synth_func and get_func are really fundamentally
similar and should be merged. That is because the distinction whether a
property value is "synthetized" or just based on a plain property is
minor. It's better to have the general concept of "convert property to
GVariant" in one form only.
Note that compare_property() is by default implemented based
on get_func. Hence, if get_func and synth_func get merged,
compare_property() will also require access to the NMConnection.
Also it makes some sense: some properties are artificial and actually
stored in "another" setting of the connection. But still, the property
descriptor for the property is in this setting. The example is the
"bond.interface-name" which only exists on D-Bus. It's stored as
"connection.interface-name".
I don't really like to say "exists on D-Bus only". It's still a valid
property, despite in NMSetting it's stored somehow differently (or not
at all). So, this is also just a regular property for which we have a
property-info vtable.
Does it make sense to compare such properties? Maybe. But the point is that
compare_property() function needs sometimes access to the entire
connection. So add the argument.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
We already need to special handle regular settings (with secrets as
GObject properties) and VPN secrets.
Next, we will also need to special handle WireGuard peers, which can
have secrets too.
Move the code to a virtual function, so that "nm-connection.c" and
"nm-setting.c" does not have explicit per-setting knowledge.
Instead of special-casing the aggregate implementation for NMSettingVpn,
delegate to a virtual function.
This will also work with other settings, that have properties/secrets
that are not GObject based properties.
And merge it with the version that uses no flags.
Previously, clear_secrets(_with_flags()) was only implemented
by NMSettingVpn. All other settings would only consider GObject-based
properties.
As we will add secrets that have no GObject property, call the virtual
function always, so that the setting can hook into this (for WireGuard
peers).
The secret name should be the one that we can pass to nm_setting_get_secret_flags().
It's wrong to call the function repeatedly with secret-name "secrets".
Probably nobody cared anyway about the name. nm_connection_clear_secrets_with_func()
is used to clear secrets based on the flags, not the secret-name.
Fixes: 2b2404bbef
Order the code in our common way. No other changes.
- ensure to include the main header first (directly after
"nm-default.h").
- reorder function definitions: get_property(), set_property(),
*_init(), *_new(), finalize(), *_class_init().
NMSetting's compare_property() has and had two callers:
nm_setting_compare() and nm_setting_diff().
compare_property() accepts a NMSettingCompareFlags argument, but
at the same time, both callers have another complex (and
inconsistent!) set of pre-checks for shortcuting the call of
compare_property(): should_compare_prop().
Merge should_compare_prop() into compare_property(). This way,
nm_setting_compare() and nm_setting_diff() has less additional
code, and are simpler to follow. Especially nm_setting_compare()
is now trivial. And nm_setting_diff() is still complicated, but
not related to the question how the property compares or whether
it should be compared at all.
If you want to know whether it should be compared, all you need to do
now is follow NMSettingClass.compare_property().
This changes function pointer NMSettingClass.compare_property(),
which is public API. However, no user can actually use this (and shall
not!), because _nm_setting_class_commit_full() etc. is private API. A
user outside of libnm-core cannot create his/her own subclasses of
NMSetting, and never could in the past. So, this API/ABI change doesn't
matter.
Secret-flags are flags, but most combinations don't actually make sense
and maybe should be rejected. Anyway, that is not done, and most places
just check that there are no unknown flags set.
Add _nm_setting_secret_flags_valid() to perform the check at one place
instead of having the implementation at various places.
We should no longer use nm_connection_for_each_setting_value() and
nm_setting_for_each_value(). It's fundamentally broken as it does
not work with properties that are not backed by a GObject property
and it cannot be fixed because it is public API.
Add an internal function _nm_connection_aggregate() to replace it.
Compare the implementation of the aggregation functionality inside
libnm with the previous two checks for secret-flags that it replaces:
- previous approach broke abstraction and require detailed knowledge of
secret flags. Meaning, they must special case NMSettingVpn and
GObject-property based secrets.
If we implement a new way for implementing secrets (like we will need
for WireGuard), then this the new way should only affect libnm-core,
not require changes elsewhere.
- it's very inefficient to itereate over all settings. It involves
cloning and sorting the list of settings, and retrieve and clone all
GObject properties. Only to look at secret properties alone.
_nm_connection_aggregate() is supposed to be more flexible then just
the two new aggregate types that perform a "find-any" search. The
@arg argument and boolean return value can suffice to implement
different aggregation types in the future.
Also fixes the check of NMAgentManager for secret flags for VPNs
(NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs
is a property that either has a secret or a secret-flag. The previous
implementation would only look at present secrets and
check their flags. It wouldn't check secret-flags that are
NM_SETTING_SECRET_FLAG_NONE, but have no secret.
There are 3 kinds of secret flag implementations:
1) regular properties have a GObject property and a corresponding
"-flags" property.
2) NMSettingVpn handles this entirely differently
3) NMSettingWirelessSecurity's WEP keys, where the secret keys
share a flags property that does not follow the same naming
scheme as 1).
The getter and setter had a boolean "verifiy_secret", only to
handle 3). Drop that parameter. Don't let NMSettingWirelessSecurity
call the parent's implementation for WEP keys. Just let it handle
it directly.
- most of the time, the secret-name is short and fits in a
stack-allocated buffer.
Optimize for that by using nm_construct_name_a().
- use _nm_utils_ascii_str_to_int64() instead of strtoul().
tmp = strtoul ((const char *) val, NULL, 10);
if ((errno != 0) || (tmp > NM_SETTING_SECRET_FLAGS_ALL)) {
is not the right way to check for errors of strtoul().
- refactor the code to return-early on errors.
- since commit 9b96bfaa72 "setting-vpn: whatever is in vpn.secrets always
is a secrets", we accept secrets without secret-flags as valid too.
However, only do that, when we at least have a corresponding key in
priv->secrets hash. If the secret name is not used at all, it's
clearly not a secret.
- if the secret flags are not a valid number, pretend that the flags
are still set to "none" (zero). That is because we use the presence
of the "*-flags" data item as indication that this is in fact a
secret. The user cannot use data items with such a name for another
purpose, so on failure, we still claim that this is in fact a secret.
We already had nm_free_secret() to clear the secret out
of a NUL terminated string. That works well for secrets
which are strings, it can be used with a cleanup attribute
(nm_auto_free_secret) and as a cleanup function for a
GBytes.
However, it does not work for secrets which are binary.
For those, we must also track the length of the allocated
data and clear it.
Add two new structs NMSecretPtr and NMSecretBuf to help
with that.
NMSetting internally already tracked a list of all proper GObject properties
and D-Bus-only properties.
Rework the tracking of the list, so that:
- instead of attaching the data to the GType of the setting via
g_type_set_qdata(), it is tracked in a static array indexed by
NMMetaSettingType. This allows to find the setting-data by simple
pointer arithmetic, instead of taking a look and iterating (like
g_type_set_qdata() does).
Note, that this is still thread safe, because the static table entry is
initialized in the class-init function with _nm_setting_class_commit().
And it only accessed by following a NMSettingClass instance, thus
the class constructor already ran (maybe not for all setting classes,
but for the particular one that we look up).
I think this makes initialization of the metadata simpler to
understand.
Previously, in a first phase each class would attach the metadata
to the GType as setting_property_overrides_quark(). Then during
nm_setting_class_ensure_properties() it would merge them and
set as setting_properties_quark(). Now, during the first phase,
we only incrementally build a properties_override GArray, which
we finally hand over during nm_setting_class_commit().
- sort the property infos by name and do binary search.
Also expose this meta data types as internal API in nm-setting-private.h.
While not accessed yet, it can prove beneficial, to have direct (internal)
access to these structures.
Also, rename NMSettingProperty to NMSettInfoProperty to use a distinct
naming scheme. We already have 40+ subclasses of NMSetting that are called
NMSetting*. Likewise, NMMetaSetting* is heavily used already. So, choose a
new, distinct name.
Previously, each (non abstract) NMSetting class had to register
its name and priority via _nm_register_setting().
Note, that libnm-core.la already links against "nm-meta-setting.c",
which also redundantly keeps track of the settings name and gtype
as well.
Re-use NMMetaSettingInfo also in libnm-core.la, to track this meta
data.
The goal is to get rid of private data structures that track
meta data about NMSetting classes. In this case, "registered_settings"
hash. Instead, we should have one place where all this meta data
is tracked. This was, it is also accessible as internal API,
which can be useful (for keyfile).
Note that NMSettingClass has some overlap with NMMetaSettingInfo.
One difference is, that NMMetaSettingInfo is const, while NMSettingClass
is only initialized during the class_init() method. Appart from that,
it's mostly a matter of taste, whether we attach meta data to
NMSettingClass, to NMMetaSettingInfo, or to a static-array indexed
by NMMetaSettingType.
Note, that previously, _nm_register_setting() was private API. That
means, no user could subclass a functioning NMSetting instance. The same
is still true: NMMetaSettingInfo is internal API and users cannot access
it to create their own NMSetting subclasses. But that is almost desired.
libnm is not designed, to be extensible via subclassing, nor is it
clear why that would be a useful thing to do. One day, we should remove
the NMSetting and NMSettingClass definitions from public headers. Their
only use is subclassing the types, which however does not work.
While libnm-core was linking already against nm-meta-setting.c,
nm_meta_setting_infos was unreferenced. So, this change increases
the binary size of libnm and NetworkManager (1032 bytes). Note however
that roughly the same information was previously allocated at runtime.
- Don't use @parent_class name. This local variable (and @object_class) is
the class instance up-cast to the pointer types of the parents. The point
here is not that it is the direct parent. The point is, that it's the
NMSettingClass type.
Also, it can only be used inconsistently, in face of NMSettingIP4Config,
who's parent type is NMSettingIPConfig. Clearly, inside
nm-setting-ip4-config.c we wouldn't want to use the "parent_class"
name. Consistently rename @parent_class to @setting_class.
- Also rename the pointer to the own class to @klass. "setting_class" is also the
wrong name for that, because the right name would be something like
"setting_6lowpan_class".
However, "klass" is preferred over the latter, because we commonly create new
GObject implementations by copying an existing one. Generic names like "klass"
and "self" inside a type implementation make that simpler.
- drop useless comments like
/* virtual functions */
/* Properties */
It's better to logically and visually structure the code, and avoid trival
remarks about that. They only end up being used inconsistently. If you
even need a stronger visual separator, then an 80 char /****/ line
should be preferred.
Add a new option that allows to activate a profile multiple times
(at the same time). Previoulsy, all profiles were implicitly
NM_SETTING_CONNECTION_MULTI_CONNECT_SINGLE, meaning, that activating
a profile that is already active will deactivate it first.
This will make more sense, as we also add more match-options how
profiles can be restricted to particular devices. We already have
connection.type, connection.interface-name, and (ethernet|wifi).mac-address
to restrict a profile to particular devices. For example, it is however
not possible to specify a wildcard like "eth*" to match a profile to
a set of devices by interface-name. That is another missing feature,
and once we extend the matching capabilities, it makes more sense to
activate a profile multiple times.
See also https://bugzilla.redhat.com/show_bug.cgi?id=997998, which
previously changed that a connection is restricted to a single activation
at a time. This work relaxes that again.
This only adds the new property, it is not used nor implemented yet.
https://bugzilla.redhat.com/show_bug.cgi?id=1555012
constructor functions are ugly, because code is running before
main() starts. Instead, as the registration code for NMSetting types
is insid the GType constructor, we just need to ensure at the
right place, that the GType was created.
The right place here is _register_settings_ensure_inited(), because
that is called before we need the registration information.
XXX was used to either raise attention (NOTE) or to indicate
that this is ugly code that should be fixed (FIXME). The usage
was inconsistent.
Let's avoid XXX and use either NOTE or FIXME.
