NetworkManager will reject scan requests, if it is currently scanning.
That is very wrong. Even if NetworkManager wants to ratelimit scan
requests or not scan at critical moments, it should never reject a
request, but remember and start scanning as soon as it can.
That should be fixed.
But regardless, also nmcli can do better.
If you issue
$ nmcli device wifi list --rescan yes
once, it works as expected.
If you issue it again right after, the scan request of nmcli will be
rejected. But nmcli cannot just merely complete and print the result.
Instead, it will wait in the hope that a scan result will be present
soon. But if the request was simply rejected, then the result will
never come, and nmcli hangs for the 15 seconds timeout.
Instead, repeatedly re-trigger scan requests, in the hope that as soon
as possible we will be ready.
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.
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
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.
In all the cases, we don't want to perform locale dependent comparison.
$ sed -i 's/\<strcasecmp\>/g_ascii_\0/g' $(git grep -w -l strcasecmp -- ':(exclude)shared/systemd/' )
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
Add a new 'carrier' flag to the InterfaceFlags property of devices to
indicate the current carrier state.
The new flag is equivalent to the 'lower-up' flag for all devices
except the ones that use a non-standard carrier detection mechanism
like NMDeviceAdsl.
Otherwise repeated "nmcli d wifi hotspot" commands create multiple
Hostpot connections, which is just sad. We do already reuse existing
connections with "nmcli d wifi connect" -- let's just do a similar thing
here.
A quick overview of the currently connected Wi-Fi network, including
credentials. Comes handy if someone wants to connect more devices to
their Hotspot or the same network as they are connected to.
In a future commit it will be useful to know whether the activation
details when the activation succeeds.
This also makes the state tracking of the ongoing activation more
elegant, since we got our device and AC neatly packed together and we
can treat their respective state changes consistently.
We're dereferencing the info pointer in the argument list in the call to
nm_client_activate_connection_async(). Stealing it at that point causes
a crash.
This reverts a chunk of commit b298f2e605 ('cli: use cleanup macro for
freeing AddAndActivateInfo').
We should prefer the cleanup macors nm_auto*() because they express
ownership in code.
Also, they allow to return early without additional cleanup code.
That way we can refactor if-else blocks.
Also, in cases where we intentionally pass on the reference, we use
g_steal_pointer(), which literally spells out what happens in code.
If we find a matching connection, ensure it's exactly as we want it
before actually proceeding to activate it. Fixes this problem:
# nmcli dev wifi connect "Network of Doom" password santa <-- bad
Error: Connection activation failed: (7) Invalid secrets
# nmcli dev wifi connect "Network of Doom" password satan <-- correct
Error: Connection activation failed: (7) Invalid secrets
The password is now correct, but nmcli chose to re-activate the wrong
connection it created previously.
Do not check for password when creating a simple connection object for
"nmcli dev wifi connect".
This makes no difference in practice. The password is checked for
existence later on and the connection instance is created anyway. This
just makes things look a bit more consistent.
Instead of straight-out rejecting extra parameters for various nmcli
sub-commands (such as "nmcli dev status", "nmcli dev wifi rescan" or
"nmcli dev wifi connect", etc.), we just print a warning and go ahead.
This is unhelpful. In case the user makes a typo, we'll do the wrong
thing and possibly even drown the warning in the output.
While at that, let's make the error message consistent. One less string
to translate.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/217
Since commit 62b939de4e ('cli: add nmc_complete_strv() which takes a
string array for completion that may contain NULL'), the sentinel is
no longer needed.
Call the correct _finish() function for
nm_client_add_and_activate_connection_async(). add_and_activate_cb()
somewhat confusingly alternates between two different ones depending on
whether info->create is set.
Fixes: 3593237527 ('cli: reuse connections in nmcli dev wifi con')
https://github.com/NetworkManager/NetworkManager/pull/326
