Previously, if @active referenced a device but was not currently queued
or the current activation request, nothing was done.
Now, in such a case still call nm_active_connection_set_state_fail().
Note that nm_active_connection_set_state_fail() has no effects on
active-connections that are already in disconnected state (which
we would expect by such an active connection). Likely there is no
visible change here, but it feels more correct to ensure the active
connection is always failed.
nm_device_disconnect_active_connection() is generally useful and a prefered
form to fail an active connection. The device's state-change reason is important,
so it needs to be injected.
The names "floating" and "sink()" are well known and good.
However, "disarm()" seems the best name for the counterpart operation,
better than "float()", "unsink()", or "freeze()".
Since we have "nm_keep_alive_disarm()", for consitency rename
- "floating" -> (not) "armed"
- "sink()" -> "arm()"
All callers only want to create floating instances at first.
Also, it seems to make generally more sense this way: you create
a floating instance, set it up, and then arm it.
This simplifies nm_keep_alive_new(), which previously was adding
additional code that wasn't accessible via plain g_object_new().
The NMKeepAlive instance is useful to find out when we should disconnect.
The moment when we start disconnecting, we don't care about it anymore.
Add a nm_keep_alive_disarm() function to suppress property change events about
the alive state, after that point. Emitting further events from that point
on is only confusing.
Yes, this means, a NMKeepAlive instance shall not be re-used for
multiple purposes. Create a separate keep-alive instace for each target
that should be guarded.
Also, once disarmed, we can release all resources that the NMKeepAlive instance
was holding until now.
remove_device() is also called when the device has no longer a valid
ifindex and so device_is_wake_on_lan() must do an extra check to avoid
the following assertion:
nmp_cache_lookup_entry_link: assertion 'ifindex > 0' failed
0 _g_log_abort () from target:/lib64/libglib-2.0.so.0
1 g_logv () from target:/lib64/libglib-2.0.so.0
2 g_log () from target:/lib64/libglib-2.0.so.0
3 nmp_cache_lookup_entry_link (cache=0xb858f0, ifindex=ifindex@entry=0) at ../src/platform/nmp-object.c:1713
4 nmp_cache_lookup_link (cache=<optimized out>, ifindex=ifindex@entry=0) at ../src/platform/nmp-object.c:1728
5 nm_platform_link_get_obj (self=self@entry=0xb85840, ifindex=ifindex@entry=0, visible_only=visible_only@entry=1) at ../src/platform/nm-platform.c:759
6 nm_platform_link_get (self=self@entry=0xb85840, ifindex=ifindex@entry=0) at ../src/platform/nm-platform.c:784
7 nm_platform_link_get_type (self=self@entry=0xb85840, ifindex=ifindex@entry=0) at ../src/platform/nm-platform.c:1065
8 link_get_wake_on_lan (platform=0xb85840, ifindex=0) at ../src/platform/nm-linux-platform.c:6963
9 nm_platform_link_get_wake_on_lan (self=self@entry=0xb85840, ifindex=0) at ../src/platform/nm-platform.c:1705
10 device_is_wake_on_lan (platform=0xb85840, device=<optimized out>) at ../src/nm-manager.c:1617
11 remove_device (self=0xbd0060, device=<optimized out>, device@entry=0xd298c0, quitting=quitting@entry=0, allow_unmanage=allow_unmanage@entry=1)
12 device_removed_cb (device=0xd298c0, user_data=0xbd0060) at ../src/nm-manager.c:1698
13 _g_closure_invoke_va () from target:/lib64/libgobject-2.0.so.0
14 g_signal_emit_valist () from target:/lib64/libgobject-2.0.so.0
15 g_signal_emit () from target:/lib64/libgobject-2.0.so.0
16 available_connections_check_delete_unrealized_on_idle (user_data=0xd298c0) at ../src/devices/nm-device.c:4446
Fixes: ca3bbede74
Since commit 945c904f95 "platform: assert against valid ifindex and
remove duplicate assertions", it is no longer allowed to call certain
platform functions with invalid ifindex.
These trigger now an assertion. Note that the assertion is merely a
g_return_val_if_fail(), hence in non-debug mode, this does not lead to
a crash.
Fixes: 945c904f95
The problem is that updating the metered value of a shared connection is
not implemented. The user needs to fully reactivate the profile for changes
to take effect.
That is unfortunate, especially because reapplying the route metric
works in other other cases.
Having a NMCmdLine implementation here is wrong.
For one, it local to nm-dnsmasq-manager.c and not reusable.
If there is anything of value in such an implementation, then it should
possibly also be useful at other places that create command line
arguments.
Note that in the end, command line arguments are just strv arrays.
There are different ways how to construct that strv array. For example,
do we need to clone the strings that we add? How to do that most
elegantly and efficiently? The previous implementation for example used a
GStringChunk for that (quite creative!). The point is, there are pros and
cons about how to create strv arrays. But constructing command line options
shouldn't be abstracted in a NMCmdLine API. It should use a suitable API
for creating an strv array. Otherwise, it's too much abstraction.
Drop NMCmdLine and use GPtrArray directly. Together with a few helper
functions nm_strv_ptrarray_*() that is our preferred way to create such
strv arrays. Is it perfect? No, we still g_strdup() static strings.
That could be optimized. But then we would want an optimized API for
constructing strv arrays, not NMCmdLine.
These are simple macros/functions that append a heap-allocated string to
a GPtrArray. It is intended for a GPtrArray which takes ownership of the
strings (meaning, it has a g_free() GDestroyNotify).
Checking whether the link exists in the cache, before talking to kernel
serves no purpose.
- in all cases, the caller already has a good indication that the link
in fact exists. That is, because the caller makes decisions on what to
do, based on what platform told it earlier. Thus, the check usually succeeds
anyway.
- in the unexpected case it doesn't succeed, we
- should not silently return without logging at least a message
- we possibly still want to send the netlink message to kernel,
just to have it fail. Note that the ifindex is indeed the identifier
for the link, so there is no danger of accidentally killing the
wrong link.
Well, theoretically there is, because the kernel's ifindex counter can
wrap or get reused when moving links between namespaces. But checking
the cache would not protect against that anyway! Worst case, the cache
would already have the impostor link and would not prevent from doing
the wrong thing. After all, they do have the same identifier, so how
would we know that this is in fact a different link?
We want that all code paths assert strictly and gracefully.
That means, if we have function nm_platform_link_get() which calls
nm_platform_link_get_obj(), then we don't need to assert the same things
twice. Don't have the calling function assert itself, if it is obvious
that the first thing that it does, is calling a function that itself
asserts the same conditions.
On the other hand, it simply indicates a bug passing a non-positive
ifindex to any of these platform functions. No longer let
nm_platform_link_get_obj() handle negative ifindex gracefully. Instead,
let it directly pass it to nmp_cache_lookup_link(), which eventually
does a g_return_val_if_fail() check. This quite possible enables
assertions on a lot of code paths. But note that g_return_val_if_fail()
is graceful and does not lead to a crash (unless G_DEBUG=fatal-criticals
is set for debugging).
nm_platform_link_delete() will soon assert against positive ifindex
argument.
nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME));
will result in an assertion, if the link does not exist.
Extend nmtstp_link_delete() to gracefully skip deleting the link
so that it can be used in such situations.
Also, rename nmtstp_link_del() to nmtstp_link_delete(), because it's
closer to nm_platform_link_delete().
We want to assert for valid input arguments, but we don't want
multiple assertions for the same.
Move the assertion from nm_platform_link_get() to
nm_platform_link_get_obj().
That way, nm_platform_link_get_obj() also checks the input arguments.
At the same time, nm_platform_link_get() gets simpler and still does
the same amount of assertions.
In nmp_cache_lookup_link_full(), we may have multiple candidates that match.
Continue searching, until we find a visible one. That way, visible results
are preferred.
Note that for links, nmp_object_is_visible() checks whether the link is
visible in netlink (instead of only udev).
This code will be used later.
We want to remember which keyfiles are currently loaded (or hidden).
With the addition or multiple keyfile directories (soon), there are
two cases where this matters:
- if there are multiple keyfiles which reference the same UUID,
we can only load one of them. That is already a problem today
with only one keyfile directory, where multiple files can reference
the same UUID.
The implementation will pick the file based on priorities (like
the file modification date). However, the user may call explicitly
call `nmcli connection load`. In that case, we cannot reload
all files to find out whether the to be loaded file is hidden
according to the defined priorities. We cannot do that, because we
must not make decisions based on files on disk, which we are not told
to reload. So, during a `nmcli connection load` we must look at
unrelated files, to determine how to load the file.
Instead, we do allow the user to load any file, even if it would be
shadowed by other files. When we do that, we may want to persist which
file is currently loaded, so that a service restart and a `nmcli connection
reload` does not undo the load again. This can be later later be solved by
writing a symlink
"/var/run/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile"
which targets the currently active file.
- if a profile was loaded from read-only persistant storage, the user
may still delete the profile. We also need to remember the deletion
of the file. That will be achieved by symlinking "/dev/null" as
"/etc/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile".
Add helper functions to read and write these symlinks.
VALUES_STATIC() was a macro to initialize the values_static pointer with
a (static) strv array.
For one, it lacked a "const" in "(const char *[])", which means
the data is not put in a read only section by the linker. That should
be fixed.
Anyway, we already have a macro for creating such constant strv arrays:
NM_MAKE_STRV().
I think it is good to the concept of "initializing values_static" a
name (VALUES_STATIC()). But it also hides (for better or worse), that
this is a strv array. Let's use NM_MAKE_STRV() instead. By looking at
the code, it's still clear that this initializes the "values_static"
array, but it also makes it clear that this is a plain strv array.
From [1]:
You may optionally specify attribute names with ‘__’ preceding and
following the name. This allows you to use them in header files
without being concerned about a possible macro of the same name. For
example, you may use the attribute name __noreturn__ instead of
noreturn.
[1] https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax
Add a new CON_DEFAULT() macro that places a property name into a
special section used at runtime to check whether it is a supported
connection default.
Unfortunately, this mechanism doesn't work for plugins so we have to
enumerate the connection defaults from plugins in the daemon using
another CON_DEFAULT_NOP() macro.
Supporting a trailing comma in NM_MAKE_STRV() can be desirable, because it
allows to extend the code with less noise in the diff.
Now, there may or may not be a trailing comma at the end.
There is a downside to this: the following no longer work:
const char *const v1[] = NM_MAKE_STRV ("a", "b");
const char *const v2[3] = NM_MAKE_STRV ("a", "b");
but then, above can be written more simply already as:
const char *const v1[] = { "a", "b", NULL };
const char *const v2[3] = { "a", "b" };
so the fact that the macro won't work in that case may be preferable,
because it forces you to use the already existing better variant.
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.
There's no reason the mesh shouldn't autoconnect. Almost.
The mesh and regular Wi-Fi shares the same radio. There, in the first
place, probably shouldn't have been separate NMDevices. Not sure whether
we can fix it at this point, but we can surely avoid unnecessary
competition between the two devices: give the regular Wi-Fi priority and
only connect mesh if the regular companion stays disconnected.
For the record; connections shipped on XO-1 laptops all have
autoconnect=off and thus are not affected by this.
If the client wants to pinpoint the connection to a particular device
they can just add an appropriate property.
That said, MAC address probably even doesn't count as appropriate; an
interface name is supposed to stay stable and could be used in such
cases.
This fixes the case where "nmcli d wifi connect ..." ends up with a
connection tied to a rather random device that happened to be around
even without the "ifname" argument.
