Veth interfaces should be shown as Ethernet from
nm_device_get_type_description in order to provide backward
compatibility.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
In commit 201c153e25 ('libnm: fix GObject Introspection annotations
for functions returning a GPtrArray') these annotations were changed
to fix Vala bindings. However, bindings may treat the transfer
annotation for GPtrArray differently, so depending on the binding
we either get a leak or a double free.
It's unclear how to fix that. For now, just add a warning to the
documentation to avoid it.
The following reproducer leads to a crash:
#!/bin/python
import gi
gi.require_version("NM", "1.0")
from gi.repository import NM
def _pr(msg):
NM.utils_print(0, msg + "\n")
def process(nmc):
for device in nmc.get_devices():
cons = device.filter_connections(nmc.get_connections())
_pr(
"device %s (%s) has %s compatible connections"
% (device.get_iface(), NM.Object.get_path(device), len(cons))
)
process(NM.Client.new())
See-also: https://gitlab.gnome.org/GNOME/gobject-introspection/-/issues/305
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
With 1.22, various synchronous functions for invoking D-Bus methods were
deprecated. The reason was that D-Bus is fundamentally asynchronous, and
providing synchronous API in NMClient is inherently wrong. That is
because NMClient essentially is a cache of the D-Bus API, and invoking
g_dbus_connection_call_sync() messes up the order of events from D-Bus.
In particular, when the synchronous function completes, the content of
the cache does not yet reflect the change.
Since they got deprecated, the question is with what to replace them.
Instead of adding a (e.g.) nm_client_networking_set_enabled_async()
for nm_client_networking_set_enabled(), just expect the user to call
D-Bus directly.
D-Bus itself defines a reasonable API, and with GDBusConnection it
is fine (and convenient) to just call D-Bus operations directly.
Often libraries try to abstract D-Bus by providing convenience
wrappers around D-Bus API. I think that often is wrong and unnecessary.
Note that libnm's NMClient does a lot more than just wrapping simple
D-Bus calls. It provides a complete client-side cache of the D-Bus
interface. As such, what libnm's NMClient does is more than simple
wrappers around D-Bus. NMClient is a reasonable thing to do.
However, it is unnecessary to add API like nm_client_networking_set_enabled_async()
that only calls g_dbus_connection_call(). Don't pretend that we would need such
trivial wrappers in libnm.
Instead, recommend to use g_dbus_connection_call(). Or alternatively,
the convenience wrappers nm_client_dbus_call() and
nm_client_dbus_set_property().
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.
Add VRF support to the daemon. When the device we are activating is a
VRF or a VRF's slave, put routes in the table specified by the VRF
connection.
Also, introduce a VRF device type in libnm.
I have a coredump that seems to indicate that nm_device_get_active_connection()
did not return a valid object. Let's add an assertion, trying to identify the
issue earlier. Aside from that, this change isn't useful, but an nm_assert()
shouldn't hurt anyway.
No longer use GDBusObjectMangaerClient and gdbus-codegen generated classes
for the NMClient cache. Instead, use GDBusConnection directly and a
custom implementation (NMLDBusObject) for caching D-Bus' ObjectManager
data.
CHANGES
-------
- This is a complete rework. I think the previous implementation was
difficult to understand. There were unfixed bugs and nobody understood
the code well enough to fix them. Maybe somebody out there understood the
code, but I certainly did not. At least nobody provided patches to fix those
issues. I do believe that this implementation is more straightforward and
easier to understand. It removes a lot of layers of code. Whether this claim
of simplicity is true, each reader must decide for himself/herself. Note
that it is still fairly complex.
- There was a lingering performance issue with large number of D-Bus
objects. The patch tries hard that the implementation scales well. Of
course, when we cache N objects that have N-to-M references to other,
we still are fundamentally O(N*M) for runtime and memory consumption (with
M being the number of references between objects). But each part should behave
efficiently and well.
- Play well with GMainContext. libnm code (NMClient) is generally not
thread safe. However, it should work to use multiple instances in
parallel, as long as each access to a NMClient is through the caller's
GMainContext. This follows glib's style and effectively allows to use NMClient
in a multi threaded scenario. This implies to stick to a main context
upon construction and ensure that callbacks are only invoked when
iterating that context. Also, NMClient itself shall never iterate the
caller's context. This also means, libnm must never use g_idle_add() or
g_timeout_add(), as those enqueue sources in the g_main_context_default()
context.
- Get ordering of messages right. All events are consistently enqueued
in a GMainContext and processed strictly in order. For example,
previously "nm-object.c" tried to combine signals and emit them on an
idle handler. That is wrong, signals must be emitted in the right order
and when they happen. Note that when using GInitable's synchronous initialization
to initialize the NMClient instance, NMClient internally still operates fully
asynchronously. In that case NMClient has an internal main context.
- NMClient takes over most of the functionality. When using D-Bus'
ObjectManager interface, one needs to handle basically the entire state
of the D-Bus interface. That cannot be separated well into distinct
parts, and even if you try, you just end up having closely related code
in different source files. Spreading related code does not make it
easier to understand, on the contrary. That means, NMClient is
inherently complex as it contains most of the logic. I think that is
not avoidable, but it's not as bad as it sounds.
- NMClient processes D-Bus messages and state changes in separate steps.
First NMClient unpacks the message (e.g. _dbus_handle_properties_changed()) and
keeps track of the changed data. Then we update the GObject instances
(_dbus_handle_obj_changed_dbus()) without emitting any signals yet. Finally,
we emit all signals and notifications that were collected
(_dbus_handle_changes_commit()). Note that for example during the initial
GetManagedObjects() reply, NMClient receive a large amount of state at once.
But we first apply all the changes to our GObject instances before
emitting any signals. The result is that signals are always emitted in a moment
when the cache is consistent. The unavoidable downside is that when you receive
a property changed signal, possibly many other properties changed
already and more signals are about to be emitted.
- NMDeviceWifi no longer modifies the content of the cache from client side
during poke_wireless_devices_with_rf_status(). The content of the cache
should be determined by D-Bus alone and follow what NetworkManager
service exposes. Local modifications should be avoided.
- This aims to bring no API/ABI change, though it does of course bring
various subtle changes in behavior. Those should be all for the better, but the
goal is not to break any existing clients. This does change internal
(albeit externally visible) API, like dropping NM_OBJECT_DBUS_OBJECT_MANAGER
property and NMObject no longer implementing GInitableIface and GAsyncInitableIface.
- Some uses of gdbus-codegen classes remain in NMVpnPluginOld, NMVpnServicePlugin
and NMSecretAgentOld. These are independent of NMClient/NMObject and
should be reworked separately.
- While we no longer use generated classes from gdbus-codegen, we don't
need more glue code than before. Also before we constructed NMPropertiesInfo and
a had large amount of code to propagate properties from NMDBus* to NMObject.
That got completely reworked, but did not fundamentally change. You still need
about the same effort to create the NMLDBusMetaIface. Not using
generated bindings did not make anything worse (which tells about the
usefulness of generated code, at least in the way it was used).
- NMLDBusMetaIface and other meta data is static and immutable. This
avoids copying them around. Also, macros like NML_DBUS_META_PROPERTY_INIT_U()
have compile time checks to ensure the property types matches. It's pretty hard
to misuse them because it won't compile.
- The meta data now explicitly encodes the expected D-Bus types and
makes sure never to accept wrong data. That would only matter when the
server (accidentally or intentionally) exposes unexpected types on
D-Bus. I don't think that was previously ensured in all cases.
For example, demarshal_generic() only cared about the GObject property
type, it didn't know the expected D-Bus type.
- Previously GDBusObjectManager would sometimes emit warnings (g_log()). Those
probably indicated real bugs. In any case, it prevented us from running CI
with G_DEBUG=fatal-warnings, because there would be just too many
unrelated crashes. Now we log debug messages that can be enabled with
"LIBNM_CLIENT_DEBUG=trace". Some of these messages can also be turned
into g_warning()/g_critical() by setting LIBNM_CLIENT_DEBUG=warning,error.
Together with G_DEBUG=fatal-warnings, this turns them into assertions.
Note that such "assertion failures" might also happen because of a server
bug (or change). Thus these are not common assertions that indicate a bug
in libnm and are thus not armed unless explicitly requested. In our CI we
should now always run with LIBNM_CLIENT_DEBUG=warning,error and
G_DEBUG=fatal-warnings and to catch bugs. Note that currently
NetworkManager has bugs in this regard, so enabling this will result in
assertion failures. That should be fixed first.
- Note that this changes the order in which we emit "notify:devices" and
"device-added" signals. I think it makes the most sense to emit first
"device-removed", then "notify:devices", and finally "device-added"
signals.
This changes behavior for commit 52ae28f6e5 ('libnm: queue
added/removed signals and suppress uninitialized notifications'),
but I don't think that users should actually rely on the order. Still,
the new order makes the most sense to me.
- In NetworkManager, profiles can be invisible to the user by setting
"connection.permissions". Such profiles would be hidden by NMClient's
nm_client_get_connections() and their "connection-added"/"connection-removed"
signals.
Note that NMActiveConnection's nm_active_connection_get_connection()
and NMDevice's nm_device_get_available_connections() still exposes such
hidden NMRemoteConnection instances. This behavior was preserved.
NUMBERS
-------
I compared 3 versions of libnm.
[1] 962297f908, current tip of nm-1-20 branch
[2] 4fad8c7c64, current master, immediate parent of this patch
[3] this patch
All tests were done on Fedora 31, x86_64, gcc 9.2.1-1.fc31.
The libraries were build with
$ ./contrib/fedora/rpm/build_clean.sh -g -w test -W debug
Note that RPM build already stripped the library.
---
N1) File size of libnm.so.0.1.0 in bytes. There currently seems to be a issue
on Fedora 31 generating wrong ELF notes. Usually, libnm is smaller but
in these tests it had large (and bogus) ELF notes. Anyway, the point
is to show the relative sizes, so it doesn't matter).
[1] 4075552 (102.7%)
[2] 3969624 (100.0%)
[3] 3705208 ( 93.3%)
---
N2) `size /usr/lib64/libnm.so.0.1.0`:
text data bss dec hex filename
[1] 1314569 (102.0%) 69980 ( 94.8%) 10632 ( 80.4%) 1395181 (101.4%) 1549ed /usr/lib64/libnm.so.0.1.0
[2] 1288410 (100.0%) 73796 (100.0%) 13224 (100.0%) 1375430 (100.0%) 14fcc6 /usr/lib64/libnm.so.0.1.0
[3] 1229066 ( 95.4%) 65248 ( 88.4%) 13400 (101.3%) 1307714 ( 95.1%) 13f442 /usr/lib64/libnm.so.0.1.0
---
N3) Performance test with test-client.py. With checkout of [2], run
```
prepare_checkout() {
rm -rf /tmp/nm-test && \
git checkout -B test 4fad8c7c64 && \
git clean -fdx && \
./autogen.sh --prefix=/tmp/nm-test && \
make -j 5 install && \
make -j 5 check-local-clients-tests-test-client
}
prepare_test() {
NM_TEST_REGENERATE=1 NM_TEST_CLIENT_BUILDDIR="/data/src/NetworkManager" NM_TEST_CLIENT_NMCLI_PATH=/usr/bin/nmcli python3 ./clients/tests/test-client.py -v
}
do_test() {
for i in {1..10}; do
NM_TEST_CLIENT_BUILDDIR="/data/src/NetworkManager" NM_TEST_CLIENT_NMCLI_PATH=/usr/bin/nmcli python3 ./clients/tests/test-client.py -v || return -1
done
echo "done!"
}
prepare_checkout
prepare_test
time do_test
```
[1] real 2m14.497s (101.3%) user 5m26.651s (100.3%) sys 1m40.453s (101.4%)
[2] real 2m12.800s (100.0%) user 5m25.619s (100.0%) sys 1m39.065s (100.0%)
[3] real 1m54.915s ( 86.5%) user 4m18.585s ( 79.4%) sys 1m32.066s ( 92.9%)
---
N4) Performance. Run NetworkManager from build [2] and setup a large number
of profiles (551 profiles and 515 devices, mostly unrealized). This
setup is already at the edge of what NetworkManager currently can
handle. Of course, that is a different issue. Here we just check how
long plain `nmcli` takes on the system.
```
do_cleanup() {
for UUID in $(nmcli -g NAME,UUID connection show | sed -n 's/^xx-c-.*:\([^:]\+\)$/\1/p'); do
nmcli connection delete uuid "$UUID"
done
for DEVICE in $(nmcli -g DEVICE device status | grep '^xx-i-'); do
nmcli device delete "$DEVICE"
done
}
do_setup() {
do_cleanup
for i in {1..30}; do
nmcli connection add type bond autoconnect no con-name xx-c-bond-$i ifname xx-i-bond-$i ipv4.method disabled ipv6.method ignore
for j in $(seq $i 30); do
nmcli connection add type vlan autoconnect no con-name xx-c-vlan-$i-$j vlan.id $j ifname xx-i-vlan-$i-$j vlan.parent xx-i-bond-$i ipv4.method disabled ipv6.method ignore
done
done
systemctl restart NetworkManager.service
sleep 5
}
do_test() {
perf stat -r 50 -B nmcli 1>/dev/null
}
do_test
```
[1]
Performance counter stats for 'nmcli' (50 runs):
456.33 msec task-clock:u # 1.093 CPUs utilized ( +- 0.44% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
5,900 page-faults:u # 0.013 M/sec ( +- 0.02% )
1,408,675,453 cycles:u # 3.087 GHz ( +- 0.48% )
1,594,741,060 instructions:u # 1.13 insn per cycle ( +- 0.02% )
368,744,018 branches:u # 808.061 M/sec ( +- 0.02% )
4,566,058 branch-misses:u # 1.24% of all branches ( +- 0.76% )
0.41761 +- 0.00282 seconds time elapsed ( +- 0.68% )
[2]
Performance counter stats for 'nmcli' (50 runs):
477.99 msec task-clock:u # 1.088 CPUs utilized ( +- 0.36% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
5,948 page-faults:u # 0.012 M/sec ( +- 0.03% )
1,471,133,482 cycles:u # 3.078 GHz ( +- 0.36% )
1,655,275,369 instructions:u # 1.13 insn per cycle ( +- 0.02% )
382,595,152 branches:u # 800.433 M/sec ( +- 0.02% )
4,746,070 branch-misses:u # 1.24% of all branches ( +- 0.49% )
0.43923 +- 0.00242 seconds time elapsed ( +- 0.55% )
[3]
Performance counter stats for 'nmcli' (50 runs):
352.36 msec task-clock:u # 1.027 CPUs utilized ( +- 0.32% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
4,790 page-faults:u # 0.014 M/sec ( +- 0.26% )
1,092,341,186 cycles:u # 3.100 GHz ( +- 0.26% )
1,209,045,283 instructions:u # 1.11 insn per cycle ( +- 0.02% )
281,708,462 branches:u # 799.499 M/sec ( +- 0.01% )
3,101,031 branch-misses:u # 1.10% of all branches ( +- 0.61% )
0.34296 +- 0.00120 seconds time elapsed ( +- 0.35% )
---
N5) same setup as N4), but run `PAGER= /bin/time -v nmcli`:
[1]
Command being timed: "nmcli"
User time (seconds): 0.42
System time (seconds): 0.04
Percent of CPU this job got: 107%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.43
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 34456
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 6128
Voluntary context switches: 1298
Involuntary context switches: 1106
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
[2]
Command being timed: "nmcli"
User time (seconds): 0.44
System time (seconds): 0.04
Percent of CPU this job got: 108%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.44
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 34452
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 6169
Voluntary context switches: 1849
Involuntary context switches: 142
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
[3]
Command being timed: "nmcli"
User time (seconds): 0.32
System time (seconds): 0.02
Percent of CPU this job got: 102%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.34
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 29196
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 5059
Voluntary context switches: 919
Involuntary context switches: 685
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
---
N6) same setup as N4), but run `nmcli monitor` and look at `ps aux` for
the RSS size.
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
[1] me 1492900 21.0 0.2 461348 33248 pts/10 Sl+ 15:02 0:00 nmcli monitor
[2] me 1490721 5.0 0.2 461496 33548 pts/10 Sl+ 15:00 0:00 nmcli monitor
[3] me 1495801 16.5 0.1 459476 28692 pts/10 Sl+ 15:04 0:00 nmcli monitor
These setters not only invoke a synchronous D-Bus call (ignoring the
return value). They also modify the content of the cache client-side,
bypassing the information that we receive via notifications from the
server.
Also, they don't emit property changed signals, but in any case they
are broken beyond repair.
Fully mark them as deprecated. Note that they were already marked as
_NM_DEPRECATED_SYNC_METHOD. However, that does not actually mark
the API as deprecated, because fully deprecating all synchronous
methods is premature at this point.
Our NMObject implementations should behave in a similar manner.
For example, string properties should be coerced the a consistent
manner.
Add functions _nml_coerce_property_*() for that. Of course, they
are trivial. Their value is not that they encapsulate some complex
implementation, but that they convey a general concept of how we
want to handle certain properties in NMClient's object cache.
Default values should preferably be zero and/or a value that indicates
that the property is unknown/unset.
In practice, this property is not unset because it's present
on the D-Bus API.
Yes, by default (server side) devices do autoconnect.
But that does not mean an NMObject, that has its GObject property
not set via D-Bus shall default to TRUE.
Default values preferably should be FALSE, because that is what we get
by default (memset(0)).
These types are all subclasses of NMObject. These instances are commonly
created by NMClient itself. It makes no sense that a user would
instantiate the type. Much less does it make sense to subclass them.
Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability. But most
importantly, we can obtain a static offset where to access the private data.
That means, we can use the information to access the data pointer
generically, as we will need later.
This is not done for the internal types NMManager, NMRemoteSettings,
and NMDnsManager. These types will be dropped later.
The majority of sources in "libnm/" are implementations of NMObject.
"nm-libnm-utils.h" will contain common definitions for handling such
objects. This means, most of the source files under libnm will require
this include. Include it by default.
This is not merely cosmetic. I will need the obj_properties
array to lookup GParamSpec by their PROP_* enum value. The
alternative would be lookup by name, which is more expensive.
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.
NM_DEVICE_MANAGED was intended to work like NM_DEVICE_AUTOCONNECT:
namely it would call the D-Bus property setter synchronously.
But such behavior is horrendous, we certainly don't want blocking calls
during a property getter.
Luckily this one instance was unused and never worked as the property
was marked as G_PARAM_READABLE. Just drop the setter.
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 built (among others) two libraries from the sources in "shared/nm-utils":
"libnm-utils-base.la" and "libnm-utils-udev.la".
It's confusing. Instead use directories so there is a direct
correspondence between these internal libraries and the source files.
(cherry picked from commit 2973d68253)
At a few places we checked whether neighbor->attrs was non-NULL.
That is not necessary, unless we'd like to catch some dangling/invalid
pointers. The attrs hash is always set otherwise.
Instead of just dropping the check, add a NM_IS_LLDP_NEIGHBOR() macro
(inline function).
For one, just reassigning copy->attrs leaks the previous
hash table. Fix that.
Also, NMLldpNeighbor instances are not immutable. I think that
is an uglyness, and it would be preferable that they can be sealed.
A sealed object could safely share/ref the internal hash-table. However,
as it is, we cannot just have two NMLldpNeighbor instances share the
same hash-table. Do a full copy.
Add the const qualifier to the attribute name in LLDP API functions so
that const strings and string literals are accepted. This change is
backwards compatible for existing users of the API.
nm_device_get_device_type would report the device type as it was send on
DBus, while fetching the property would mean that only a known device
types is reported.
Make both results consistent by coercing in nm_device_get_device_type
rather than when setting the property.
(cherry picked from commit a6a185ba00)
The device type was set to the GType rather than a new value in the
NMDeviceType enum.
Add the corresponding enum entry, fix the device type and set the
routing priority to the same value as generic devices.
(cherry picked from commit 8d9365a973)
The @connection argument can be NULL; add the (allow-none) annotation
otherwise calling the API with a NULL argument through GObject
introspection fails with:
Argument 1 does not allow None as a value
Fixes: 278fd4fb0f
