Use `_nm_connection_ensure_setting()` to eliminate the
duplicated codes. This function will retrieve the specific setting from
connection, if not found, create new one and attach to the connection.
Signed-off-by: Gris Ge <fge@redhat.com>
Commit df0dc912cc ('8021x: don't request secrets if they are empty
and system owned') changed the setting so that NM doesn't request the
PIN for PKCS#11 certificates and keys when the password property has
NM_SETTING_SECRET_FLAG_NONE. From the commit message:
Empty secrets are fine. In particular, for PKCS#11 it means that
protected authentication path is used (the secrets are obtained
on-demand from the pinpad).
This change breaks the scenario in which PINs are stored in the
connection, as the setting indicates that no secrets are required, and
thus PINs are not sent to the supplicant.
If the PIN is entered through a pinpad, users should set the secret
flags as 'not-required'.
This reverts commit df0dc912cc ('8021x: don't request secrets if
they are empty and system owned').
https://bugzilla.redhat.com/show_bug.cgi?id=1992829https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/965
For IPv4, the order is not like for IPv6. Of course not.
Fixes: 7aa4ad0fa2 ('nmcli/docs: better describe ipv[46].addresses in `man nm-settings-nmcli`')
Our handling of properties is relatively complicated. We should have
clear code paths and responsibilities who calls who.
There is from_dbus_fcn() callback to implement parsing a GVariant and
set the property in NMSetting. This is called via:
- _nm_setting_new_from_dbus()
- init_from_dbus()
- _property_set_from_dbus()
Then, one of the from_dbus_fcn() implementations is
_nm_setting_property_from_dbus_fcn_gprop(), which calls
set_property_from_dbus(). That one sets the property using GObject
setter. That's good and a clear code path.
However, set_property_from_dbus() was also called via
- _nm_setting_update_secrets()
- klass->update_one_secret()
- nm-setting.c:update_one_secret()
- set_property_from_dbus()
Meaning, there is a different code path to set_property_from_dbus(),
which bypasses from_dbus_fcn(). That is highly undesirable, because
it should be clear how a property setter gets implemented, and this
way, potentially two different implementations were used.
Refactor nm-setting.c:update_one_secret() to use
_property_set_from_dbus() instead. This behaves potentially differently
for properties like NM_SETTING_ADSL_PASSWORD, which is implemented as
a "direct" property, where from_dbus_fcn() setter no longer uses g_object_set().
This should not make a difference in practice, and in any case, now the
code paths are unified.
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.
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
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.
The aim is that properties have a "type", that is, that similar
properties share a common behavior and appearance.
Most properties of type "mac-address" normalize the string in the
GObject property setter. Three don't. Let them also do that.
This is also relevant, because the compare function for mac-addresses
(_nm_setting_property_compare_fcn_default()) converts the properties
first to a "ay" GVariant. Which means the comparison is case
insensitive. Normalizing the values in the setter avoids that
inconsistency.
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.
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.
Also, there is another change in behavior: nm_utils_ip4_dns_to_variant()
will now skip over strings that are not valid IPv4 addresses.
Previously, it would have added 0.0.0.0 (or some undefined address).
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.
