"wireguard.private-key" is special, because the setter does some unusual
normalization. To implement that, we need to use "direct_hook.set_string_func".
We want that our properties have little special cases and follow a
few common behaviors. For example, we have string properties, and those
should mostly behave the same (e.g. by being "direct-string"
properties).
That is already not fully enough, because we have slightly different
behaviors. For example, we have string properties that should have their
whitespace stripped, that should be ascii case down converted, that
should be normalized IP or MAC addresses. So far, that was expressed via
simple fields in NMSettInfoProperty, like NMSettInfoProperty's
direct_set_string_ascii_strdown field.
But that is not enough. In particular, for "wireguard.private-key" we
perform a different kind of normalization (base64 parsing, and taking
care not to leak secret in memory). It seems to special to add a boolean
flag "direct_set_string_wireguard_private_key".
Instead, add a hook that can cover that.
We need a hook, because we want one setter implementation throughout. Commonly,
we have at least two setters: the GObject set_property() and from D-Bus.
Both should call into the same underlying implementation, to avoid code
duplication. For that, the tweaked behavior must be "down", that is at
the deepest point in the call stack where we set the string. That's why
we need the hook. The alternative would be two special implementation
for GObject and D-Bus setters (and in the future we might add setters
from keyfile).
Both callers themselves needed to call _nm_setting_get_private(),
only to pass it to _property_direct_set_string().
Instead, pass the necessary parameters to _property_direct_set_string(),
so it can do that itself.
This additional parameters will be necessary when we add a hook for
setting the string.
We cache the virtual-iface-name. The caching is also part of the API as
nm_setting_infiniband_get_virtual_interface_name() returns a const
string.
As the value is computed and based on the parent and the p-key, we must
clear the cache when the parent or p-key changes (or detect that it's
invalid).
Previously, we were simply clearing the value in the set_property() function,
which is the only setter of these two properties. If we make these
properties "direct properties", then they will be directly set via
from_dbus_fcn() which bypasses the GObject setter. Which is a problem
for the cache invalidation.
We could either not make those properties direct properties. The problem
is that direct properties are nice, and they will in the future
implement further optimizations for them. Also, they are the default
implementation, and it seems clearer to build something on top of that,
instead of deviating from the default.
Instead, let the caching detect when the value needs to be regenerated.
This seems a questionable thing to do, and should be made clearer by
having a parameter (that makes you think about what is happening here).
Also, the normalization for vxlan.remote does not perform this mapping,
so the parameter is there so that the approach can handle both flavors.
Let's sprinkle some snake ointment.
This is questionable, because we copy secrets all over the place where
we their deallocation (and clearing) is not in our control. For example,
the GValue setter/getter copies the string (but does not clean the
secret). Also, when converting the property to a GVariant, we won't
clear it. So this does not catch a lot of cases.
Still, if we can with relative ease avoid leaking the string at some
places, do it.
libnm's data structures are commonly not thread safe (like
NMConnection). However, it must be possible that all operations can
operate on *different* data in a thread safe manner. That means, we need
to take care about our global variables.
nm_utils_ssid_to_utf8() uses a list of encodings, which gets cached.
- replace the GHashTables with a static list. Since it doesn't cost
anything, make the list sorted and look it up via binary search.
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.
"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`')
