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
And, while at that, add a hint to the developer adding new items. It's
helps avoid a mistake that I believe is common (because I just made it
twice...).
Most of the times we actually need a NMSecretAgentSimple typed pointer.
This way, need need to cast less.
But even if we would need to cast more, it's better to have pointers
point to the actual type, not merely to avoid shortcomings of C.
The state handling in add_and_activate_cb() and connect_device_cb() is
redundant to connected_state_cb() and in fact executed only if the
activation is really really really quick (which it never is).
Also, the return_text those implementations provide is different from
what connected_state_cb(), potentially confusing the users and adding
extra work for translators.
Not to mention extra lines of code, reading whose wastes our precious
time on the planet we could spend doing heroin instead.
Try to locate an existing connection before creating a new one when
handling "nmcli device wifi connect". This allows WPA-Enterprise
networks to be activated this way, consistent with the comment that this
command is equivalent to clicking on an SSID in a GUI client.
When nm_device_disconnect_async() returns, the device could be still
in DEACTIVATING state, and so we also register to device-state signal
notifications to know when the device state goes to DISCONNECTED.
Sometimes it happens that the device state goes to DISCONNECTED before
nm_device_disconnect_async() returns. In this case the signal handler
exits the main loop and then the callback for disconnect_async() is
executed anyway because it was already dispatched, leading to an
invalid memory access.
To avoid this we should cancel nm_device_disconnect_async() when we
are quitting the main loop.
Reproducer:
nmcli connection add type team ifname t1 con-name t1
nmcli connection up t1
nmcli device disconnect t1 & nmcli device delete t1
Crash example:
==14955==ERROR: AddressSanitizer: SEGV on unknown address 0xffffffff0000000b (pc 0x7f128c8ba3dd bp 0x0000004be080 sp 0x7ffcda7dc6e0 T0)
==14955==The signal is caused by a READ memory access.
0 0x7f128c8ba3dc in g_string_truncate (/lib64/libglib-2.0.so.0+0x713dc)
1 0x7f128c8bb4bb in g_string_printf (/lib64/libglib-2.0.so.0+0x724bb)
2 0x45bdfa in disconnect_device_cb clients/cli/devices.c:2321
3 0x7f128ca3d1a9 in g_simple_async_result_complete /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gsimpleasyncresult.c:802
4 0x7f128cf85d0e in device_disconnect_cb libnm/nm-device.c:2354
5 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
6 0x7f128ca508d5 in g_task_return /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1206
7 0x7f128ca8ecfc in reply_cb /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gdbusproxy.c:2586
8 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
9 0x7f128ca508d5 in g_task_return /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1206
10 0x7f128ca83440 in g_dbus_connection_call_done /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gdbusconnection.c:5713
11 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
12 0x7f128ca4ffac in complete_in_idle_cb /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1162
13 0x7f128c893b7a in g_idle_dispatch gmain.c:5620
14 0x7f128c89726c in g_main_dispatch gmain.c:3182
15 0x7f128c897637 in g_main_context_iterate gmain.c:3920
16 0x7f128c897961 in g_main_loop_run (/lib64/libglib-2.0.so.0+0x4e961)
17 0x473afb in main clients/cli/nmcli.c:1067
18 0x7f128c6a1412 in __libc_start_main (/lib64/libc.so.6+0x24412)
19 0x416c39 in _start (/usr/bin/nmcli+0x416c39)
https://github.com/NetworkManager/NetworkManager/pull/254https://bugzilla.redhat.com/show_bug.cgi?id=1546061
Correct the spelling across the *entire* tree, including translations,
comments, etc. It's easier that way.
Even the places where it's not exposed to the user, such as tests, so
that we learn how is it spelled correctly.
In general we want to keep the connections that the user is most likely
to want to see topmost. Default connections should be close to the top,
but the connections that are likely to have been brought up manually
shall be above them. It applies to VPN connections and should apply to
Hotspots too.
