This will also be useful for NMSecretAgentOld.
The mechanics how NMClient handles the GMainContext and the
context-busy-watcher apply really to every GObject that uses
GDBusConnection and registers to signals.
At least, as long as the API provides no shutdown/stop method,
because that means shutdown/stop happens when unreferencing the
instance, at which point pending operations get cancelled (but
they cannot complete right away due to the nature of GTask and
g_dbus_connection_call()). If there is a shutdown/stop API, then all
pending operations could keep the instance alive, and the instance
would sticks around (and keeps the GMainContext busy) until shutdown is
completed. Basically, then the instance could be the context-busy-watcher
itself.
But in existing API which does not require the user to explicitly shutdown,
that is not a feasible (backward compatible) addition. But the context-busy-watcher
object is.
The test only uses one GMainContext (the g_main_context_get_default()
singleton.
Between tests, ensure that we iterate the main context long enough,
so that no more sources from the previous test are queued. Otherwise,
there is an ugly dependency between tests and the order in which
they run.
Use nmtstc_context_object_new() to create the NMSecretAgentOld. This
randomly uses sync or async initialization, and checks whether the
main context gets iterated.
nmtstc_client_new() exists to test creating a GInitiable/GAsyncInitiable
in different GMainContext combinations.
This is not only useful for NMClient but will also be useful for
NMSecretAgentOld. Add nmtstc_context_object_new() to allow for that.
Also, allow passing parameters when creating the object.
The resulting nmtstc_context_object_new() is relatively complex. But
this is only testing code, that aims to construct the respective GObject
instance in various manners (randomly using the sync or async initialization).
It is complex, but delivers at testing various code paths of the
underlying code. The API that it provides however is simple.
Also drop _nmtstc_client_new_extra_context() to create the instance with
a different context. For one, this requires that the internal context is
integrated as long as the context-busy-watcher exists. That was not
handled correctly. Also, creating a NMClient instance with a different
context than the current thread default at construct time has
implications to the test later. The tests don't want this variant, and
don't handle them properly. So drop this.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
The device instance might already be removed from the cache. At that
point, _nm_object_get_client(self) returns %NULL.
Use the correct NMClient instance.
For printf debugging (when you recompile the source) it can be useful
to have one switch to disable logging of NMClient.
For example, this is useful with
$ LIBNM_CLIENT_DEBUG=trace nmcli agent secret
I think it's technically not correct to rely on the "sentinal" field
being immediately after the previous field, due to alignment. Implement
the macro differently.
Add a 'in-state-change' pending action to be sure the device always has a
pending when transitioning between states (this prevents callbacks to mark
startup as complete while running _set_state_full()).
This is needed as during the 'failed'->'disconnected' the pending action 'activation-*'
for the device is removed resulting in an empty pending_actions list which then
triggers 'check_if_startup_complete()' that will find no pending action and mark
startup as complete even if the device could have been activated with another connection.
https://bugzilla.redhat.com/show_bug.cgi?id=1759956
The option is mandatory in the replies from server and so we don't
need to ask for it. dhclient doesn't do it either. But especially, it
seems that requesting the option causes some broken server
implementations to send duplicate instances of the option.
So, remove the option from the parameter request list of the internal
nettools and systemd DHCP implementation.
If the server sends a packet with multiple instances of the same
option, they are concatenated during n_dhcp4_incoming_linearize() and
evaluated as a single option as per section 7 of RFC 3396.
However, there are broken server implementations that send
self-contained options in multiple copies. They are reassembled to
form a single instance by the nettools client, which then fails to
parse them because they have a length greater than the expected one.
This problem can be reproduced by starting a server with:
dnsmasq --bind-interfaces --interface veth1 -d
--dhcp-range=172.25.1.100,172.25.1.200,1m
--dhcp-option=54,172.25.1.1
In this way dnsmasq sends a duplicate option 54 (server-id) when the
client requests it in the 'parameter request list' option, as
dhcp=systemd and dhcp=nettools currently do.
While this is a violation of the RFC by the server, both isc-dhcp and
systemd-networkd client implementations have mechanisms to deal with
this situation. dhclient simply takes the first bytes of the
aggregated option. systemd-networkd doesn't follow RFC 3396 and
doesn't aggregate multiple options; it considers only the last
occurrence of each option.
Change the parsing code to accept options that are longer than
necessary.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/324
We now move the deletion of the context-busy-watcher to and idle handler
on the D-Bus GMainContext.
Note that the idle source does not take an additional reference on the
context. Hence, in certain cases it might happen that the context will
be completely unrefed before the idle handler runs. In that case, we
would leak the object.
Avoid that, by taking an additional reference to the GMainContext.
Note that the alternative would be to unref the context-busy-watcher
via the GSource's GDestroyNotify. That is not done, because then the
busy watcher might be unrefed in a different thread. Instead, we want
that to happen for the right context. The only minor downside of this
is that the user now always pays the price and must iterate the context
to fully clean up. But note that the user anyway must be prepared to
iterate the context after NMClient is gone. And that depends on some
unpredictable events that the user cannot control. That means, either
the user handles this correctly already, or the problem anyway exists
(randomly).
Of course all of the discussed "problems" are very specific. In practice, the
users uses the g_main_context_default() instance and anyway will either
keep iterating it or quit the process after the NMClient instance goes
away.
The context-busy-watch has two purposes:
1) it allows the user to watch whether the NMClient still has pending
GSource'es attached to the GMainContext.
2) thereby, it also keeps the inner GMainContext integrated into the
caller's (in case of synchronous initialization of NMClient).
Especially for 2), we must not get this wrong. Otherwise, we might
un-integrate the inner GMainContext too early and it will be leaked
indefinitely (because the user has no means to access or iterate it).
To be extra careful, extend the lifetime of the context-busy-watcher
for one more idle invocation. Theoretically, this should not be necessary,
but it's not clear whether something else is still pending.
The downside of that extra safety is that it is probably unnecessary in
practice. And in case where it is necessary, it hides an actual
issue, making it harder to notice and fix it.
It seems to complicate things more than helping. Drop it. What we still have
is a wrapper around plain g_dbus_connection_signal_subscribe(). That one is
trivial and helpful. The previous wrapper seems to add more complexity.
nm_dbus_connection_signal_subscribe_object_manager() wraps the subscription. The problem
is that this requires to pass a destroy notify function for cleaning up. Such a destroy
notify function will result in an idle source when unsubscribing, which keeps the associated
GMainContext alive (until it gets iterated some more). That seems error prone and outright
unsuitable for NMClient.
While the helper may be useful, it cannot be used by NMClient. So, there is only one
user of this function and we don't expect a second one. It seems better to get rid of
this wrapper and implement it directly.
When passing a destroy notify to g_dbus_connection_signal_subscribe(),
that callback gets invoked as an idle handler of the associated
GMainContext. That caused to have yet another source attached to the
context after the NMClient gets destroyed.
Especially with synchronous initialization of NMClient that is bad,
because we may destroy the context-busy-watcher too early. That results
in removing the integration of the inner GMainContext into the caller's
context, and thus we leak the inner context indefinitely.
Avoid that leak by not passing a cleanup function to
g_dbus_connection_signal_subscribe().
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
Just read the content once. It's wasteful to read the file twice
while parsing.
But what is worse, the file content can change any time we read it.
We make decisions based on what we read, and hence we should only
read the file once in the parsing process to get one consistent result.
Split the parsing of the file content to a separate function.
Next we will load IPv4 files only once, and thus only need to parse
the content without reading it.
Note that the function temporarily modifies the input buffer, but
restores the original content before returning.
We will need both variants, so that the caller can read the file only
once.
Note that also utils_has_route_file_new_syntax_content() will
restore the original contents and not modify the input (from point
of view of the caller). In practice it will momentarily modify the
content.
[1/5] Compiling C object 'clients/common/913ef36@@nmc-base@sta/nm-client-utils.c.o'.
../clients/common/nm-client-utils.c:528: warning: "NDEBUG" redefined
528 | #define NDEBUG
|
<command-line>: note: this is the location of the previous definition
On ppc64le, the linking fails due to unresolved symbols.
Fixes: 7d8da6c9c1 ('build: build intermediate library with core wifi for device-plugin and tests')
This property is currently most likely not used. Also, because libnm doesn't
expose it and the only known user of this API (gnome-network-displays) doesn't
use it.
In the future we may want to expand on the Groups API. E.g. exposing groups as
separate D-Bus objects, in which case a better property type would be "ao" and
not "as". For now, that is unclear nor requested.
Remove the property for now.
Groups currently are not exposed on D-Bus as separate objects.
Also, we might want to expose the property as "ao" instead of "as".
This API needs more thought.
There are likely no users that rely on this property. So, we will
drop it from server side, until it will be requested and newly designed.
Regardless, NMClient needs to gracefully ignore the property.
Despite we will remove it from 1.24 API, libnm should ignore the
property on previous versions. Mark it accordingly.
