We sometimes emit warnings after a connection is added. Currently
there's a warning when the connection ID collides with another one (and
a suggestion to use an UUID instead).
Let's move the check into a separate routine, so that we can reuse it
elsewhere, such as on connection "modify" (in a following commit).
Check if a connection uses something that is likely not to work --
either now or in future.
The ultimate decision on whether it's going to work is up to the daemon.
We just use the result to color the connection differently to provide
slight visual cue to the user.
We often create the source with default priority, no destroy function and
attach it to the default context (g_main_context_default()). For that
case, we have wrapper functions like nm_g_timeout_add_source()
and nm_g_idle_add_source(). Use those.
There should be no change in behavior.
Before, we would just ignore the errors when we passed an invalid value
to a property alias:
$ nmcli c add type ethernet mac Hello
Connection 'ethernet-1' (242eec76-7147-411a-a50b-336cf5bc8137) successfully added.
$ nmcli c show 242eec76-7147-411a-a50b-336cf5bc8137 |grep 802-3-ethernet.mac-address:
802-3-ethernet.mac-address: --
...or crash, because the GError would still be around:
$ nmcli c add type ethernet mac Hello ethernet.mac-address World
(process:734670): GLib-WARNING **: 14:52:51.436: GError set over the top of a previous GError or uninitialized memory.
This indicates a bug in someone's code. You must ensure an error is NULL before it's set.
The overwriting error message was: Error: failed to modify 802-3-ethernet.mac-address: 'World' is not a valid Ethernet MAC.
Error: failed to modify 802-3-ethernet.mac-address: 'Hello' is not a valid Ethernet MAC.
Now we catch it early enough:
$ nmcli c add type ethernet mac Hello
Error: failed to modify 802-3-ethernet.mac-address: 'Hello' is not a valid Ethernet MAC.
Fixes: 40032f4614 ('cli: fix resetting values via property alias')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1134
When using the "remove" command on nmcli edit mode it will reset the
value to the default when no property value is specified. If the
property value is specified it will remove that specific property.
Example:
```
nmcli> set ethernet.wake-on-lan phy
nmcli> print ethernet.wake-on-lan
802-3-ethernet.wake-on-lan: phy, default
nmcli> remove ethernet.wake-on-lan default
nmcli> print ethernet.wake-on-lan
802-3-ethernet.wake-on-lan: phy
nmcli> remove ethernet.wake-on-lan
nmcli> print ethernet.wake-on-lan
802-3-ethernet.wake-on-lan: default
```
This patch introduces "add" command to nmcli edit mode. When using "add"
it will append the value to the ones already set. This is doing the same
thing than the "set" command does right now.
Example:
```
nmcli> add ipv4.addresses 192.168.1.1/24
```
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
NetworkManager (the daemon) has no defined working directory, so
it can only handle absolute path names. This is in general and also for
the LoadConnections() D-Bus call.
That means, nmcli should make relative paths absolute.
We don't use g_canonicalize_filename() because that also cleans up
double slash and "/./". I don't think we should do that in this case, we
should only prepend $PWD to make the path absolute.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/794
While both functions are basically the same, the majority of the time
we use g_snprintf(). There is no strong reason to prefer one or the
other, but let's keep using one variant.
Coverity warns about this:
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.32.4/src/nmcli/agent.c:87: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.32.4/src/nmcli/agent.c:87: var_assign: Assigning: "pre_input_deftext" = storage returned from "g_strdup(secret->value)".
NetworkManager-1.32.4/src/nmcli/agent.c:87: overwrite_var: Overwriting "pre_input_deftext" in "pre_input_deftext = g_strdup(secret->value)" leaks the storage that "pre_input_deftext" points to.
# 85| /* Prefill the password if we have it. */
# 86| rl_startup_hook = set_deftext;
# 87|-> pre_input_deftext = g_strdup(secret->value);
# 88| }
# 89| if (secret->no_prompt_entry_id)
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.32.4/src/nmcli/common.c:712: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.32.4/src/nmcli/common.c:712: var_assign: Assigning: "nmc_rl_pre_input_deftext" = storage returned from "g_strdup(secret->value)".
NetworkManager-1.32.4/src/nmcli/common.c:712: overwrite_var: Overwriting "nmc_rl_pre_input_deftext" in "nmc_rl_pre_input_deftext = g_strdup(secret->value)" leaks the storage that "nmc_rl_pre_input_deftext" points to.
# 710| /* Prefill the password if we have it. */
# 711| rl_startup_hook = nmc_rl_set_deftext;
# 712|-> nmc_rl_pre_input_deftext = g_strdup(secret->value);
# 713| }
# 714| }
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.
The libreadline starting from version 6 is licensed as GPLv3. For some
use cases it is not acceptable to use this license.
In the NetworkManager the libreadline is used by nmcli.
This change allows using libedit instead of libreadline.
Following adjustments were made:
1. The history_set_history_state() is not supported in the libedit.
Instead, the where_history() with remove_history() were used to remove
the history content if needed.
2. rl_complete_with_tilde_expansion - it is the binary flag used only
when one wants to have the expansion support. The libedit is not
supporting and hence exporting this flag.
When one wants to compile the nmcli with libedit (GPLv2 replacement of
libreadline) the rl_completion_display_matches_hook hook shall be left
untouched (as NULL) as it is not supported in libedit.
$ nmcli connection add type dummy con-name x autoconnect no ipv4.method disabled ipv6.method disabled ifname d0
$ mcli connection up x ifname bogus
Connection successfully activated (D-Bus active path: /org/freedesktop/NetworkManager/ActiveConnection/12)
This is not right. A non-existing ifname argument was simply ignored
and nmcli would tell NetworkManager to activate the profile (on any
device).
Instead, if the user specifies a device argument, also for a virtual
type, it must exist.
Note that usually for virtual devices (like 'dummy'), the device
in fact does not exist, so passing `ifname` is likely to fail.
If the device already exists, then the command is no going to work
as expected, with
$ mcli connection up x ifname d0
succeeding, while
$ mcli connection up x ifname d1
fails (as intended) with
Error: device 'd1' not compatible with connection 'x': The interface names of the device and the connection didn't match..
$ nmcli connection add type ethernet con-name x autoconnect no ipv4.method disabled ipv6.method disabled
$ nmcli connection up x ifname bogus
Error: device 'bogus' not compatible with connection 'x'.
Better would be:
Error: device 'bogus' not found for connection 'x'.
Property aliases should really just be shortcuts for one fully spelled
out property (sometimes, they do more like "master").
Anyway, we must also handle resetting the value, otherwise:
$ nmcli connection add type gsm apn ""
will still result in "gsm.apn=internet", unlike
$ nmcli connection add type gsm gsm.apn ""
g_set_error(error, 1, 0, ...) is not right. "1" is not a valid GQuark,
we should initialize proper error instances.
Use nm_utils_error_set() for that.
Also, the code previously hacked the numeric value "1" to indicate
ambiguous text. Add and use a new error code NM_UTILS_ERROR_AMBIGUOUS
for that.
Review and replace usages of the two nm_connection_to_dbus() flags
marked deprecated in commit 84648e562c98 ('libnm: Refactor
NM_CONNECTION_SERIALIZE_* flags'):
NM_CONNECTION_SERIALIZE_NO_SECRETS and
NM_CONNECTION_SERIALIZE_ONLY_SECRETS.
