Many func implementations are asynchronous, that means, they
cannot return right away. Instead, they record the return value
in nmc->result_value.
The return value from the command functions was thus redundant.
In the best case, the return value agrees with the cached result
in nmc->result_value, in which it was unnecessary. In the worst case,
they disagree, and overwrite each other.
nmc->result_value is state. Tracking state is hard, and there should
be fewer places where the state gets mutated. Also, the rules how that
happened should be clearer. Drop the redundant, conflicting mechanism.
It's bad style to pass the argv argument around and mutate it.
We shouldn't mutate it, and not assume that it stays around after
the function returns to the caller (meaning, we should clone the
array if we intend to use it later).
Add const specifier.
It is useful from inside a function to know the command that it belongs to.
Currently we have do_networking_on() and do_networking_off() as separate
functions. However, these are basically the same with a minor difference.
If the func callback could know the "cmd" that it was called for, these
function can be combined.
Of course, without passing the NMCCommand instance, you still can
achieve the same results, especially as the NMCCommand instances are
static and known at compile time: just have separate func
implementations. But by passing the command to the function, they
*can* be combined, which is a useful thing to do.
- move the main func declarations to nmcli.h and give them a common
prefix "nmc_command_func_" prefix.
- remove some of the header files that are now empty. In fact, these
headers did not really declare some well separated module. While we
probably should structure the code in nmcli better with better layering,
it was not and still is not. Having these dummy headers don't mean that
the code is well structured and they serve little purpose.
- move the static NMCommand lists variables into the function scope
where they are used.
This would only be necessary when editing the test themselves,
to not only remove individual tests, but entire test runs.
It's not necessary to describe that in the howto.
When the users configure a DNS server on the interface, they likely
want to use it, regardless whether there is a default route on the
device.
For that to work, add an explicit "~" search domain.
Otherwise, by default NetworkManager only adds the special search domain
only on devices that have a "best default route" (nm_ip_config_best_default_route_is).
But that only considers a best default route in the main table, and
WireGuard (with ipx-auto-default-route) adds the default route to a
separate table. The heuristic to determine best devices works not well
with policy routing, so explicitly add this search domain during import.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/405
This is obviously a change in behavior, as we now honor backslash
escape sequences. With this change, all string values can be expressed,
both as option keys and values.
Previously, you could for example not set vpn.secrets to have a ','
and you could not set vpn.data to
nmcli connection modify "$PROFILE" +vpn.data 'ipsec-ike = aes256-sha2_256-modp2048,aes256-sha2_256-modp1536'
Use a relatively simple backslash escaping scheme. The main goal of
the scheme is that it doesn't change behavior for the majority of cases.
It only changes behavior for setting an option if:
- the string contains a backslash
- and if the backslash proceeds one of the few characters that support
escaping now (white space, ',', '\\', and '=').
The only downside here is that backslash is only treated special, if it
preceeds a character that requires escaping. That makes the behavior
non intuitive. However, it allows to write most backslashes without
escaping them as "\\\\" and thus keep previous behavior.
The nmcli getters now also escape the options accordingly. That means,
the string printed by the getter is also a valid input for the setter.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/390
The two modes VALUE_STRSPLIT_MODE_OBJLIST and VALUE_STRSPLIT_MODE_MULTILIST
basically do regular split and afterwards g_strstrip() all values and
remove empty tokens.
That is what the NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP flag already does.
Use it.
There should be no change in behavior.
We should not use global variables, and we should minimize the state
that we pass around. Instead of requiring the full NmCli struct in
nm_cli_spawn_pager(), pass only the necessary data.
This reduces our use of global variables.
Of course, we later pass the point on, where we need to cast the constness away
again. This is more a reminder that we aren't suppost to change the variable.
We should try to avoid access to global variables. For libreadline
callbacks we still need a global variable.
Introduce a global variable nm_cli_global_readline, specially for this
use. It makes the places clear where we use it, and discourages
the use at other places, where we better avoid global variables.
Otherwise, we just see "returncode: -11", which isn't very clear.
By default, our test nmcli invocations should never exit by a signal.
Usually, when we encounter a signal, it indicates to a bug and a crash
during the test.
Print more information about the exit code.
returncode: -11 (SIGNAL SIGSEGV)
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
Showcase nm_client_dbus_set_property().
Thereby, also print error messages and return an error if
the command fails.
Also, enable PolicyKit authentication (although, I think there are
some bugs with this still).
Previously, we would call the synchronous nm_client_networking_set_enabled()
method. There were 3 problems:
1) nmcli ignored the return value, that means, if the request failed with
access denied it would just silently pretend that it succeeded.
2) nmcli first called nmc_start_polkit_agent_start_try(), but when
invoking the synchronous method, the main context is busy and a
polkit request cannot possibly be handled.
3) nm_client_networking_set_enabled() is deprecated.
Fix all of these, by calling the D-Bus method directly.
Policykit authentication requests are only handled partly. There
seems to be an unrelated race/bug. Now it works sometimes.
This is more a showcase for using nm_client_dbus_call(), than a
real use.
In this case, nmcli was mostly fine to just invoke the synchronous API
and not care about the problems that it had.
Still, replace it, and show the suggested alternative.
Drop the special casing of not scanning. Now do_device_wifi_list()
always handles the scan list in a callback.
Also fix the error code for scanning for a certain "bssid", which
previously was not set if scanning was not performed:
$ nmcli device wifi list --rescan no bssid bogus
Success
cols_len might be larger than header_row->len. That is when
the cols has entries that are not leaf entries (which currently
I think is never the case).
Fix it to use the right variable for the length of the row.
It makes debugging and understanding the code slightly simpler, if we
have a pointer of correct type, instead of returning a GArray. We don't
need the GArray at this point anymore.
Refactor the selection of the Wi-Fi device by name. Avoid
find_wifi_device_by_iface() to lookup by name. We already
have a sorted list of candidate devices. The ifname is just
an additional filter to exclude devices. So, we shouldn't
use find_wifi_device_by_iface(), but instead check our prepared
list of devices, whether it contains matching candidates.
It's not really necessary, but it feels slightly more correct. The only
reason not to take a reference is to safe the overhead of increasing and
decreasing the reference counter. But that doesn't matter for nmcli
at this point (and is tiny anyway). Let the API make sure that the instances
are kept alive.
The compare pattern seems simple, but seems error prone and subtle.
NM_CMP*() avoids that.
For example, nm_access_point_get_strength() returns an uint8_t.
C will promote those values to "int" before doing the subtraction.
Likewise, nm_access_point_get_frequency() returns a uint32_t. This
gets promoted to unsigned int when doing the subtraction. Afterwards,
that is converted to a signed int.
So both cases were in fact correct. But such things are not obvious.
Also, as fallback sort by D-Bus path. While that is not semantically
useful, we should use a defined sort order.
We already have an implementation for converting a binary
array to hex. And, it doesn't require a GString for constructing
the output that has an known length.
When creating a new connection before the user gets the chance to modify
the bond options let's just initialize 'mode' and 'miimon' (with related
'updelay' and 'downdelay').
Initializing also 'arp_interval', 'arp_ip_target' and 'primary' doesn't
make much sense as by default they're disabled or contain empty values.
We use the filename of the imported .conf file for "connection.interface-name".
That follows what `wg-quick` does.
However, we also validate that the interface name is valid UTF-8
(otherwise -- as it currently is -- the setting couldn't be send via
D-Bus). As such, we have stricter requirements.
We want to fail early and tell the user when the filename is unsuitable.
Failing later gives a worse user experience, because the failure message
about invalid "connection.interface-name" wouldn't make it clear that
the filename is wrong.
Use the appropriate function to validate "connection.interface-name".
Before:
$ touch $'./a\344b.conf'
$ nmcli connection import type wireguard file $'./a\344b.conf'
Error: failed to import './a?b.conf': Failed to create WireGuard connection: connection.interface-name: 'a?b': interface name must be UTF-8 encoded.
Now:
$ nmcli connection import type wireguard file $'./a\344b.conf'
Error: failed to import './a?b.conf': The name of the WireGuard config must be a valid interface name followed by ".conf".
