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.
tools/test-networkmanager-service.py is our NetworkManager stub server.
NetworkManager uses libnm(-core) heavily, for example to decide whether
a connection verifies (nm_connection_verify()) and for normalizing
connections (nm_connection_normalize()).
If the stub server wants to mimic NetworkManager, it also must use these
function. Luckily, we already can do so, by loading libnm using python
GObject introspection.
We already correctly set GI_TYPELIB_PATH search path, so that the
correct libnm is loaded -- provided that we build with introspection
enabled.
We still need to gracefully fail, if starting the stub server fails.
That requries some extra effort. If the stub server notices that
something is missing, it shall exit with status 77. That will cause
the tests to g_test_skip().
Coccinelle:
@@
expression a, b;
@@
-a ? a : b
+a ?: b
Applied with:
spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.
Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
The D-Bus interface already has a boolean property "Unsaved".
While that is nicer too look at (in the API), adding a new flag
is very cumbersome, and also has more overhead. For example,
it requires extending the D-Bus API, all the way down to libnm.
Add a flags argument, that will allow to add future boolean
flags easier.
Use the same form everywhere: "TRANSLATORS" instead of "Translators".
The manual also seems to prefer the upper-case form [1].
$ sed 's/\<Translators\>: /TRANSLATORS: /g' $(git grep -l Translators) -i
[1] https://www.gnu.org/software/gettext/manual/gettext.html
There are multiple tests with the same in different directories; add a
unique prefix to test names so that it is clear from the output which
one is running.
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.
Note that this changes API for checkpoint_create_async() in Python
via GIR. Previously it would require an integer argument, now a flags
argument. But this API is still unstable, it will be introduced with
1.12.
Now that the D-Bus signals in server are reordered, creating
a checkpoint in libnm crashes:
$ examples/python/gi/checkpoint.py create 4
#0 0x00007ffff6d011ee in __strcmp_sse2_unaligned () at /lib64/libc.so.6
#1 0x00007fffeb611c90 in find_checkpoint_info (manager=manager@entry=0x5555559e5110 [NMManager], path=0x7fffdc0092f0 "/org/freedesktop/NetworkManager/Checkpoint/6")
at libnm/nm-manager.c:153
#2 0x00007fffeb611d8f in checkpoint_added (manager=0x5555559e5110 [NMManager], checkpoint=checkpoint@entry=0x555555a122d0 [NMCheckpoint]) at libnm/nm-manager.c:1194
#3 0x00007fffef7db929 in g_cclosure_marshal_VOID__OBJECTv (closure=0x5555559e4b30, return_value=<optimized out>, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, param_types=0x5555559e2fc0) at gmarshal.c:2102
#4 0x00007fffef7d8976 in _g_closure_invoke_va (closure=0x5555559e4b30, return_value=0x0, instance=0x5555559e5110, args=0x7fffffffc1c8, n_params=1, param_types=0x5555559e2fc0)
at gclosure.c:867
#5 0x00007fffef7f3ff4 in g_signal_emit_valist (instance=instance@entry=0x5555559e5110, signal_id=signal_id@entry=97, detail=0, var_args=var_args@entry=0x7fffffffc1c8) at gsignal.c:3300
#6 0x00007fffef7f4b48 in g_signal_emit_by_name (instance=instance@entry=0x5555559e5110, detailed_signal=detailed_signal@entry=0x7fffffffc310 "checkpoint-added") at gsignal.c:3487
#7 0x00007fffeb6156d1 in deferred_notify_cb (data=0x5555559e5110) at libnm/nm-object.c:219
#8 0x00007fffeb615ae7 in object_property_maybe_complete (self=0x5555559e5110 [NMManager]) at libnm/nm-object.c:555
#9 0x00007fffeb615e5d in object_created (obj=<optimized out>, path=<optimized out>, user_data=<optimized out>) at libnm/nm-object.c:576
#10 0x00007fffeb61648b in handle_object_array_property (pi=<optimized out>, value=0x7fffdc075070, property_name=0x7fffeb67f117 "checkpoints", self=0x5555559e5110 [NMManager])
at libnm/nm-object.c:671
#11 0x00007fffeb61648b in handle_property_changed (self=self@entry=0x5555559e5110 [NMManager], dbus_name=<optimized out>, value=<optimized out>) at libnm/nm-object.c:740
#12 0x00007fffeb6166e9 in properties_changed (proxy=<optimized out>, changed_properties=<optimized out>, invalidated_properties=<optimized out>, user_data=0x5555559e5110)
at libnm/nm-object.c:772
...
That is, because NetworkManager now first emits signals that the checkpoint
object was created, before answering the D-Bus request. That makes more
sense, but leads to this crash.
The ugliness of how libnm handles object visibility is considerable.
libnm hides objects until they are fully initialized. So, when
the async create-checkpoint operation returns, the object might not
yet be ready to be exposed. We need to delay the result. It would be
better if the API would simply return the created path.
It generates the following warning:
libnm/nm-autoptr.h:25: Error: NM: identifier not found on the first line:
* Note that you might use this header with older versions of libnm
Fixes: ff8e563365
"nm-autoptr.h" is done in a way that allows you to copy the header
in your source tree to support older versions of libnm, that didn't
contain the header yet. For example, we might want to use it in
network-manager-applet, but we don't want to bump the libnm dependency
to 1.11.2+ only to get this functionality.
Note that G_DEFINE_AUTOPTR_CLEANUP_FUNC() was added in glib 2.43.4,
and requires compiler support for the cleanup attribute. The compiler
support is taken as given, because we rely on it already. However,
NetworkManager and network-manager-applet still don't depend on a glib
version recent enough to provide these macros. To actually use them
(*inside*) NetworkManager/network-manager-applet, we either would have
to bump the glib minimal dependency, or reimplement g_autoptr in
/shared/nm-utils/nm-glib.h compat header.
https://bugzilla.gnome.org/show_bug.cgi?id=794294
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
../libnm/tests/test-general.c: In function ‘test_fixup_vendor_string’:
../libnm/tests/test-general.c:70:3: error: initializer element is not constant
T_DATA ("3Com", "3Com"),
^
../libnm/tests/test-general.c:70:3: error: (near initialization for ‘data[0]’)
../libnm/tests/test-general.c: In function ‘test_fixup_product_string’:
../libnm/tests/test-general.c:365:3: error: initializer element is not constant
T_DATA ("10/100BaseTX [RTL81xx]", "RTL81xx"),
...
Fixes: 817fce917b
CC libnm/tests/libnm_tests_test_general-test-general.o
libnm/tests/test-general.c: In function ‘test_fixup_product_string’:
libnm/tests/test-general.c:328:1: note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without
test_fixup_product_string (void)
^~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: 8e32d8fc23
It's very likely that the product said something that was filtered out
by the fixup, such as "PCI Ethernet" or "Wi-Fi Adapter". Use a generic
type name in place of it.
The product names are generally of rather poor quality. The product name
is no place to enumerate product capabilities, the bus it's attached on
and similar nonsense.
If there's a [<string>] that survived the substitution, then the string
is supposed to be a short form that is generally preferrable.
That's great in theory, but actually it's rather often pure garbage for
product names. Let's prefer it just for vendors and provide an option to
drop it (will be useful for fixing up product names).
The hwdb generally contains the strings of rather poor quality,
especially when it comes to sensibly presenting them to the user and
they need various cleanups.
While the following patches add fixups, this one splits out vendor
fixups, because it turns out that a different set of fixups is needed
than for products.
We commonly only allow tabs at the beginning of a line, not
afterwards. The reason for this style is so that the code
looks formated right with tabstop=4 and tabstop=8.
XXX was used to either raise attention (NOTE) or to indicate
that this is ugly code that should be fixed (FIXME). The usage
was inconsistent.
Let's avoid XXX and use either NOTE or FIXME.
Otherwise, the only way to disconnect the NMVpnServicePlugin
instance is by completely unrefing it. However, often it is
not so easy to ensure that nobody else is still keeping the
instance alive, after the point where we no longer want to
handle D-Bus requests. nm_vpn_service_plugin_shutdown() to the
rescue.
We also unconditionally use them with autotools.
Also, the detection for have_version_script does
not seem correct to me. At least, it didn't work
with clang.
