"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}()".
Currently kernel only support one VLAN per VF. This must be specified in
the methods documentation.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
nm_vpn_plugin_info_new_from_file() may fail as NMVpnPlugin is an
GInitable. As such, the destructor must handle the case where the
instance was only partly initialized.
#0 g_logv (log_domain=0x7f7144703071 "GLib", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=<optimized out>) at ../glib/gmessages.c:1413
#1 0x00007f71446b3903 in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at ../glib/gmessages.c:1451
#2 0x000056455b8e58d0 in finalize (object=0x7f7128008180 [NMVpnPluginInfo]) at src/libnm-core-impl/nm-vpn-plugin-info.c:1280
#3 0x00007f71447b8b18 in g_object_unref (_object=<optimized out>) at ../gobject/gobject.c:3524
#4 g_object_unref (_object=0x7f7128008180) at ../gobject/gobject.c:3416
#5 0x00007f714486bc09 in g_initable_new_valist
(object_type=<optimized out>, first_property_name=0x56455b925c20 "filename", var_args=var_args@entry=0x7ffe702b1140, cancellable=cancellable@entry=0x0, error=error@entry=0x7ffe702b1248) at ../gio/ginitable.c:250
#6 0x00007f714486bcad in g_initable_new
(object_type=<optimized out>, cancellable=cancellable@entry=0x0, error=error@entry=0x7ffe702b1248, first_property_name=first_property_name@entry=0x56455b925c20 "filename")
at ../gio/ginitable.c:162
#7 0x000056455b8e69f6 in nm_vpn_plugin_info_new_from_file
(filename=filename@entry=0x56455c951ec0 "/opt/test/lib/NetworkManager/VPN/nm-openvpn-service.name", error=error@entry=0x7ffe702b1248) at src/libnm-core-impl/nm-vpn-plugin-info.c:1221
#8 0x000056455b88ce9a in vpn_dir_changed
(monitor=monitor@entry=0x7f7128007860 [GInotifyFileMonitor], file=file@entry=0x7f712c005600, other_file=other_file@entry=0x0, event_type=<optimized out>, user_data=<optimized out>)
at src/core/vpn/nm-vpn-manager.c:182
#9 0x00007f71448697a3 in _g_cclosure_marshal_VOID__OBJECT_OBJECT_ENUMv
(closure=0x56455c7e4250, return_value=<optimized out>, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, param_types=0x56455c7355a0) at ../gio/gmarshal-internal.c:1380
Fixes: d6226bd987 ('libnm: add NMVpnPluginInfo class')
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.
The formatting produced by clang-format depends on the version of the
tool. The version that we use is the one of the current Fedora release.
Fedora 34 recently updated clang (and clang-tools-extra) from version
12.0.0 to 12.0.1. This brings some changes.
Update the formatting.
printf() is not guaranteed to properly handle NULL string,
although glibc will print "(null)".
Avoid that by not printing the currently set value. The error
message is anyway already very long.
These type-specific getters are not very useful. _nm_connection_get_setting() is
better because the setting type is a parameter so they can be used more generically.
Have less code and use generic helpers.
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().
