Note that D-Bus is fundamentally asynchronous. Doing blocking calls
on top of D-Bus is odd, especially for libnm's NMClient. That is because
NMClient essentially is a client-side cache of the objects from the D-Bus
interface. This cache should be filled exclusively by (asynchronous) D-Bus
events (PropertiesChanged). So, making a blocking D-Bus call means to wait
for a response and return it, while queuing all messages that are received
in the meantime.
Basically there are three ways how a synchronous API on NMClient could behave:
1) the call just calls g_dbus_connection_call_sync(). This means
that libnm sends a D-Bus request via GDBusConnection, and blockingly
waits for the response. All D-Bus messages that get received in the
meantime are queued in the GMainContext that belongs to NMClient.
That means, none of these D-Bus events are processed until we
iterate the GMainContext after the call returns. The effect is,
that NMClient (and all cached objects in there) are unaffected by
the D-Bus request.
Most of the synchronous API calls in libnm are of this kind.
The problem is that the strict ordering of D-Bus events gets
violated.
For some API this is not an immediate problem. Take for example
nm_device_wifi_request_scan(). The call merely blockingly tells
NetworkManager to start scanning, but since NetworkManager's D-Bus
API does not directly expose any state that tells whether we are
currently scanning, this out of order processing of the D-Bus
request is a small issue.
The problem is more obvious for nm_client_networking_set_enabled().
After calling it, NM_CLIENT_NETWORKING_ENABLED is still unaffected
and unchanged, because the PropertiesChanged signal from D-Bus
is not yet processed.
This means, while you make such a blocking call, NMClient's state
does not change. But usually you perform the synchronous call
to change some state. In this form, the blocking call is not useful,
because NMClient only changes the state after iterating the GMainContext,
and not after the blocking call returns.
2) like 1), but after making the blocking g_dbus_connection_call_sync(),
update the NMClient cache artificially. This is what
nm_manager_check_connectivity() does, to "fix" bgo#784629.
This also has the problem of out-of-order events, but it kinda
solves the problem of not changing the state during the blocking
call. But it does so by hacking the state of the cache. I think
this is really wrong because the state should only be updated from
the ordered stream of D-Bus messages (PropertiesChanged signal and
similar). When libnm decides to modify the state, there may be already
D-Bus messages queued that affect this very state.
3) instead of calling g_dbus_connection_call_sync(), use the
asynchronous g_dbus_connection_call(). If we would use a sepaate
GMainContext for all D-Bus related calls, we could ensure that
while we block for the response, we iterate that internal main context.
This might be nice, because all events are processed in order and
after the blocking call returns, the NMClient state is up to date.
The are problems however: current blocking API does not do this,
so it's a significant change in behavior. Also, it might be
unexpected to the user that during the blocking call the entire
content of NMClient's cache might change and all pointers to the
cache might be invalidated. Also, of course NMClient would invoke
signals for all the changes that happen.
Another problem is that this would be more effort to implement
and it involves a small performance overhead for all D-Bus related
calls (because we have to serialize all events in an internal
GMainContext first and then invoke them on the caller's context).
Also, if the users wants this behavior, they could implement it themself
by running libnm in their own GMainContext. Note that libnm might
have bugs to make that really working, but that should be fixed
instead of adding such synchrnous API behavior.
Read also [1], for why blocking calls are wrong.
[1] https://smcv.pseudorandom.co.uk/2008/11/nonblocking/
So, all possible behaviors for synchronous API have severe behavioural
issues. Mark all this API as deprecated. Also, this serves the purpose of
identifying blocking D-Bus calls in libnm.
Note that "deprecated" here does not really mean that the API is going
to be removed. We don't break API. The user may:
- continue to use this API. It's deprecated, awkward and discouraged,
but if it works, by all means use it.
- use asynchronous API. That's the only sensible way to use D-Bus.
If libnm lacks a certain asynchronous counterpart, it should be
added.
- use GDBusConnection directly. There really isn't anything wrong
with D-Bus or GDBusConnection. This deprecated API is just a wrapper
around g_dbus_connection_call_sync(). You may call it directly
without feeling dirty.
---
The only other remainging API is the synchronous GInitable call for
NMClient. That is an entirely separate beast and not particularly
wrong (from an API point of view).
Note that synchronous API in NMSecretAgentOld, NMVpnPluginOld and
NMVpnServicePlugin as not deprecated here. These types are not part
of the D-Bus cache and while they have similar issues, it's less severe
because they have less state.
It should be possible to add a profile with autoconnect blocked form the
start. Update2() has a %NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT flag to
block autoconnect, and so we need something similar when adding a connection.
As the existing AddConnection() and AddConnectionUnsaved() API is not
extensible, add AddConnection2() that has flags and room for additional
arguments.
Then add and implement the new flag %NM_SETTINGS_ADD_CONNECTION2_FLAG_BLOCK_AUTOCONNECT
for AddConnection2().
Note that libnm's nm_client_add_connection2() API can completely replace
the existing nm_client_add_connection_async() call. In particular, it
will automatically prefer to call the D-Bus methods AddConnection() and
AddConnectionUnsaved(), in order to work with server versions older than
1.20. The purpose of this is that when upgrading the package, the
running NetworkManager might still be older than the installed libnm.
Anyway, so since nm_client_add_connection2_finish() also has a result
output, the caller needs to decide whether he cares about that result.
Hence it has an argument ignore_out_result, which allows to fallback to
the old API. One might argue that a caller who doesn't care about the
output results while still wanting to be backward compatible, should
itself choose to call nm_client_add_connection_async() or
nm_client_add_connection2(). But instead, it's more convenient if the
new function can fully replace the old one, so that the caller does not
need to switch which start/finish method to call.
https://bugzilla.redhat.com/show_bug.cgi?id=1677068
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
We named the types inconsistently:
- "p2p-wireless" ("libnm-core/nm-setting-p2p-wireless.h")
- "p2p" ("libnm/nm-p2p-peer.h")
- "p2p-wifi" ("src/devices/wifi/nm-device-p2p-wifi.h")
It seems to me, "libnm/nm-p2p-peer.h" should be qualified with a "Wi-Fi"
specific name. It's not just peer-to-peer, it's Wi-Fi P2P.
Yes, there is an inconsistency now, because there is already
"libnm/nm-access-point.h".
It seems to me (from looking at the internet), that the name "Wi-Fi P2P"
is more common than "P2P Wi-Fi" -- although both are used. There is also
the name "Wi-Fi Direct". But it's not clear which name should be
preferred here, so stick to "Wi-Fi P2P".
In this first commit only rename the files. The following commit will
rename the content.
Add a "a{sv}" output argument to "AddAndActivate2" D-Bus API.
"AddAndActivate2" replaces "AddAndActivate" with more options.
It also has a dictionary argument to be forward compatible so that we
hopefully won't need an "AddAndActivate3". However, it lacked a similar
output dictionary. Add it for future extensibility. I think this is
really to workaround a shortcoming of D-Bus, which does provide strong
typing and type information about its API, but does not allow to extend
an existing API in a backward compatible manner. So we either resort to
Method(), Method2(), Method3() variants, or a catch-all variant with a
generic "a{sv}" input/output argument.
In libnm, rename "nm_client_add_and_activate_connection_options()" to
"nm_client_add_and_activate_connection2()". I think libnm API should have
an obvious correspondence with D-Bus API. Or stated differently, if
"AddAndActivateOptions" would be a better name, then the D-Bus API should
be renamed. We should prefer one name over the other, but regardless
of which is preferred, the naming for D-Bus and libnm API should
correspond.
In this case, I do think that AddAndActivate2() is a better name than
AddAndActivateOptions(). Hence I rename the libnm API.
Also, unless necessary, let libnm still call "AddAndActivate" instead of
"AddAndActivate2". Our backward compatibility works the way that libnm
requires a server version at least as new as itself. As such, libnm
theoretically could assume that server version is new enough to support
"AddAndActivate2" and could always use the more powerful variant.
However, we don't need to break compatibility intentionally and for
little gain. Here, it's easy to let libnm also handle old server API, by
continuing to use "AddAndActivate" for nm_client_add_and_activate_connection().
Note that during package update, we don't restart the currently running
NetworkManager instance. In such a scenario, it can easily happen that
nmcli/libnm is newer than the server version. Let's try a bit harder
to not break that.
Changes as discussed in [1].
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/37#note_79876
"bind" specifically binds the lifetime of the activation (NMActiveConnection).
In combination with "persist=volatile", the lifetime of the NMSettingsConnection
is indirectly bound to the NMActiveConnection. But still these concepts make sense
independently.
In the future, it may make sense to also bind the lifetime of the NMSettingsConnection
to the D-Bus client. Hence, rename the option to allow for the distinction.
Also, belatedly fix libnm comment about "bind" only working with
"persist" "volatile".
Fixes: eb883e34a5
Synchronous D-Bus calls seems harmful to me, such API should not be
added to libnm. As such, all API is by default and preferably "_async".
Don't add an "_async" suffix. While we are not consistent in libnm about
this, I think for new code we should.
This adds the new methods nm_client_add_and_activate_connection_options_*
and ports the existing methods to use the new AddAndActivateConnection2
call rather than AddAndActivateConnection, allowing further parameters
to be passed in.
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
Add a test which runs nmcli against our stub NetworkManager
service and compares the output.
The output formats of nmcli are complicated and not easily understood.
For example how --mode tabular|multiline interacts with selecting
output-fields (--fields) and output modes ([default]|--terse|--pretty).
Also, there are things like `nmcli connection show --order $FIELD_SPEC`.
We need unit tests to ensure that we don't change the output
accidentally.
The libnm API fir checkpoints was only introduced with 1.11. It
is not yet stable, so there is still time to adjust it. Note that
this changes API/ABI of the development branch.
Changes:
- we only add async variants of the checkpoint functions. I believe
that synchronous D-Bus methods are fundamentally flawed, because
they mess up the ordering of events.
Rename the async functions by removing the "_async" suffix. This
matches glib style, for which the async form is also not specially
marked.
- for function that refere to a particular checkpoint (rollback and
destroy), accept the D-Bus path as string, instead of an NMCheckpoint
instance. This form is more flexible, because it allows to use
the function without having a NMCheckpoint instance at hand. On the
other hand, if one has a NMCheckpoint instance, he can trivially
obtain the path to make the call.
This allows to adjust the timeout of an existing checkpoint.
The main usecase of checkpoints, is to have a fail-safe when
configuring the network remotely. By allowing to reset the timeout,
the user can perform a series of actions, and keep bumping the
timeout. That way, the entire series is still guarded by the same
checkpoint, but the user can start with short timeout, and
re-adjust the timeout as he goes along.
The libnm API only implements the async form (at least for now).
Sync methods are fundamentally wrong with D-Bus, and it's probably
not needed. Also, follow glib convenction, where the async form
doesn't have the _async name suffix. Also, accept a D-Bus path
as argument, not a NMCheckpoint instance. The libnm API should
not be more restricted than the underlying D-Bus API. It would
be cumbersome to require the user to lookup the NMCheckpoint
instance first, especially since libnm doesn't provide an efficient
or convenient lookup-by-path method. On the other hand, retrieving
the path from a NMCheckpoint instance is always possible.
If an operation is cancelled through the GCancellable, then the idiom is
that the operation is always cancelled, even if it has finished
successfully. To ensure this is the case, add calls to
g_simple_async_result_set_check_cancellable everywhere.
Without this, e.g. gnome-control-center will crash when switching away
from the power panel quickly, as the NMClient creation finishes
asynchronously and g-c-c assume that G_IO_ERROR_CANCELLED is returned to
ensure it doesn't access the now invalid user_data parameter.
https://bugzilla.gnome.org/show_bug.cgi?id=794088
The new device type represents a PPP interface, and will implement the
activation of new-style PPPoE connections, i.e. the ones that don't
claim the parent device.
