NetworkManager/NetworkManager!1127

Vojtech Bubela authored Mar 16, 2022 and Thomas Haller committed Apr 02, 2022

sysfs is old so when bridge options are modified changes will
be sent to kernel via netlink message instead.

Vojtech Bubela authored Mar 16, 2022 and Thomas Haller committed Apr 02, 2022

Since updating options of bridge is now done with netlink we need
support function for that in nm-platform.

Vojtech Bubela authored Feb 28, 2022 and Thomas Haller committed Apr 02, 2022

Move initialization of NMSettingBridge from NMPlatformLnkBridge to separate
function.

This is needed because this initialization will be used in more than one
function.

We store the timestamp when a profile activated the last time to
"/var/lib/NetworkManager/timestamps". There was also a timer which
would update the timestamp of activated connections every 300 seconds.

That seems unnecessary, drop it.

For one, waking up every 5 minutes and rewriting a file to disk seems
undesirable, for example if /var is a device where unnecessary writes
should be minimized.

Note that we already update the timestamp when a device goes down,
and of course when it comes up. Updating the timestamp in between seems
unnecessary.

This reverts commit 60735029 ('core: update timestamp in active
system connections every 5 mins (bgo #583756)').

An alternative would be to only update the timestamp in memory (so that
it would appear updated on D-Bus), but delay writing the file until
something important happens. `nm_key_file_db_*()` already tracks whether
there are changes ("dirty") and whether it's necessary to write the
file. It would be possible to track two dirty flags: one that requires
immediate update, and one that only ensures we will re-write dirty files
eventually.

See-also: https://bugzilla.gnome.org/show_bug.cgi?id=583756

NetworkManager/NetworkManager!1171

NetworkManager/NetworkManager!1174

Previously, autotools would detect whether we have "black"
in the path. And if so, it would check formatting during `make check`.

That's problematic. When I run `./contrib/fedora/rpm/build_clean.sh -w test`
in certain cases, it would pick up black, but then fail with

  Traceback (most recent call last):
    File "/usr/bin/black", line 5, in <module>
      from black import patched_main
    File "/usr/lib/python3.6/site-packages/black.py", line 42, in <module>
      from attr import dataclass, evolve, Factory
  ModuleNotFoundError: No module named 'attr'
  make[3]: *** [Makefile:21658: check-python-black] Error 1

That's an installation error of black, but still, during package build
there is no need to check the formatting. We could export
`NMTST_SKIP_PYTHON_BLACK=1` to prevent it, but it's still unnecessary.

We check proper formatting in gitlab-ci. That is enough, it doesn't
need to run during `make check`. In particular, because `black .`
takes 1.5 seconds on my machine.

This is more for completeness, to go along "nm-code-format.sh"
script.

Usually it's very simple to run black directly (you may still do that).
However, black by default only reformats files with ".py" extension.
So to get all our python files, you'd need to know and explicitly
select them... or use this script.

Also, `black .` scans the entire source tree, and is rather slow.
This script knows which files to select and is thus faster.

gogogogi authored Mar 31, 2022 and Thomas Haller committed Apr 01, 2022

NetworkManager/NetworkManager!1172

IPv6 gets disabled at the beginning of the activation in
ip_ifindex_changed_cb(). Enable it again when the modem returns a IPv6
configuration.

Fixes: 58287cbc ('core: rework IP configuration in NetworkManager using layer 3 configuration')

NetworkManager/NetworkManager#944
NetworkManager/NetworkManager!1170

When configuring a OVS bridge to use the netdev datapath, OpenvSwitch
will create a tun interface named ovs-netdev. This interface should be
ignored by NetworkManager.

NetworkManager/NetworkManager!1169

  $ make -C po update-po

During the test, we `tee` the output to a log file in "/tmp".
We do that, because the test script cleans the working directory
several times, so the file cannot reside there.

Afterwards, we need to move the file back into the git-tree, so that
gitlab can archive it.

Previously that was done by "after_script", but the "after_script" may not
see the same "/tmp" as the test run ([1]). This needs to be done as part of the
"script" step.

[1] https://docs.gitlab.com/ee/ci/yaml/#after_script

NetworkManager/NetworkManager!1155

Our convention is that a function that fails MUST set the GError output.
No need to check for that in nm_crypto_is_pkcs12_data(). Simplify the
error paths.

Also, in gnutls' _nm_crypto_verify_pkcs12(), don't call gnutls_pkcs12_deinit()
before gnutls_strerror(). It's unclear whether that couldn't set a
different error reason.

It has no actual dependency on the crypto library. All it does, is
to be careful about not leaking secrets in memory. We have code
for that in libnm-glib-aux already. Move.

The goal is to reduce the number of places where we use libnm-crypto,
because that has a large dependency. libnm-glib-aux is a very light
dependency instead.

libnm-core is also used by the daemon, thus currently dragging in
libnm-crypto there. But could we ever drop that dependency?

One use of the libnm-crypto is in functions like nm_utils_file_is_certificate()
in "nm-utils.h". These are part of the public API of libnm.

But this is not used by the daemon. Move it to "libnm-client-core"
to be closer to where it's actually used.

As we have unit tests in "libnm-core-impl/tests" that test this function,
those unit tests also would need to move to "libnm-client-impl".
Instead, add the actual implementation of these function to "libnm-crypto"
and test it there.

This patch moves forward declarations from public header "nm-utils.h" to
"nm-client.h". Arguably, "nm-client.h" is not a great name, but we don't
have a general purpose header in "libnm-client-public", so use this.
Note that libnm users can only include <NetworkManager.h> and including
individual files is not supported (and even prevented). Thus moving
the declarations won't break any users.

libnm-core currently has a dependency on crypto libraries (either
"gnutls", "nss" or "null"). We need this huge dependency for few cases.

Move the crypto code to a separate static library"src/libnm-crypto/libnm-crypto.la".
The reasoning is that it becomes clearer where we have this dependency,
to use it more consciously, and to be better see how it's used.

We clearly need the crypto functionality in libnm. But do we also need
it in the daemon? Could we ever link the daemon without crypto libraries?

The goal of splitting the crypto part out, to better understand the
crypto dependency.

"nm-error.h" is public API of libnm, and contains error numbers and
quarks. Clearly our "nm-crypto" implementation wants to use those
errors.

I want to move "nm-crypto" out of libnm, and as it's more basic, I think
it should not have a dependency on all of libnm-core. Also because
libnm-core currently uses nm-crypto, so there would be a circular
dependency. Which would be possible to do (libnm-core-aux-intern is
also used in such a way). But it's better avoided, to have clear
hierarchy of dependencies.

Add a version of the same error codes to libnm-base. libnm-base is a
very basic dependency (just one step above libnm-glib-aux).

The goal is to make "nm-crypto.c" independent of libnm-core.

nm_utils_bin2hexstr() is part of public libnm API.
That means, if we want to use this function, we need to link with
libnm-core-impl.

This is used by "nm-crypto.c". That file is currently part of
libnm-core, but that will change.

Move the implementation to libnm-glib-aux, so that we can use this code
from all our glib-based code (because all our glib-based code is allowed
to link with libnm-glib-aux).

"autotools" also prints a similar output. It's useful to know
which libraries were enabled. Because, we run unit test against
all enabled libraries, even if they are actually used.

NetworkManager/NetworkManager!1167

If the certificate is not a blob, it makes no sense to call
nm_crypto_is_pkcs12_data().

When a static function only has one caller, it is often simpler to not
have the code in a separate function. Drop need_private_key_password()
and move it to need_secrets_tls().

g_warning() for unexpected scheme is not right. Either, this should be an
assertion (and never be hit), or the library should be silent about conditions
that can happen regularly.

I think code is easier to understand, if the difference (between phase1
and phase2) is pushed to the bottom. Having one large "if(phase2){}else{}"
at the top makes it harder to compare the two branches and see where
they differ.

The checks are duplicated and verbose. Combine them.

NetworkManager/NetworkManager!1157
https://bugzilla.redhat.com/show_bug.cgi?id=1996918

When no radio hardware is present in the system, "nmcli radio"
currently displays:

  WIFI-HW  WIFI     WWAN-HW  WWAN
  enabled  enabled  enabled  enabled

which is misleading. Use the new RadioFlags property to display
"missing" in the *-HW columns when there is no hardware for the
given radio technology.

https://bugzilla.redhat.com/show_bug.cgi?id=1996918