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>
The advantage is that the API is now the same for IPv4 and IPv6: it's
all nm_dhcp_config_*() and we can (easier) treat the address family
generically.
We still need two distinct GObject types, mainly because of the
glue code for exposing the object on D-Bus as NMDBusObject. Of course,
that could be solved differently, but as it is, it's quite nice.
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.
Remove the call-id from the requests hash before invoking the callback.
This prevents the user to cancel the request from within the callback.
Supporting such a use case is not necessary so prevent it and tighten
the callers up.
- drops nm_dispatcher_init(), which was called early in the main loop
and created a proxy synchronously. Instead, the GDBusConnection is
always ready.
- reuse the GDBusConnection of NMDBusManager. This means, we won't even
try from in "initrd" configure-and-quit mode.
- as before, there is no "manager" instance. Instead, the data is stored
in a global variable. That's ok. What is not OK is how the entire
shutdown is handled (calling dispatcher blockingly, not waiting for
requests to complete). That is fixable, but a lot of work. It is
independent of whether we use a manager object or not.
A guint value can wrap, so we would need to check that we don't allocate duplicate
IDs (which we currently don't, and it's likely never to actually hit).
Just expose the (opaque) pointer of the call-id.
We still keep a "request_id", but that is only for logging purpose.
This way, we avoid code duplication of how to print the request-id
("(%u) "), but it also will allow up to attach the interface name
and connection name to the call-id, so that we can use it for structured
logging.
We don't need the two callers both unpack the GVariant and pass a GVariantIter.
Let dispatcher_results_process() do the unpacking.
Also, use some cleanup attributes.
The monitors have been in place since the dispatcher has been introduced.
They need the daemon to do extra work know where the files are supposed to
be. It seems to me the complexity is not worth it.
Let's remove them now, making it easier to modify the dispatcher to look
for scripts in other places.
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
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.
Essentially, nm_connection_get_path() mirros nm_dbus_object_get_path().
However, when cloning a simple-connection, the path also gets cloned.
I think this field doesn't belong to NMConnection in the first place,
because NMConnection is not a D-Bus object. NMSettingsConnection (in
core) and NMRemoteConnection (in libnm) is.
Don't use the misleading alias, but use nm_dbus_object_get_path()
directly.
The fake states still encode whether the device have a default-route.
So, they are not entirely useless. Also, don't add special handling
of "#if !WITH_CONCHECK" where we don't need it.
There is no fundamental difference between compiling without connectivity
check and disabling connectivity checks.
Previously, we used the generated GDBusInterfaceSkeleton types and glued
them via the NMExportedObject base class to our NM types. We also used
GDBusObjectManagerServer.
Don't do that anymore. The resulting code was more complicated despite (or
because?) using generated classes. It was hard to understand, complex, had
ordering-issues, and had a runtime and memory overhead.
This patch refactors this entirely and uses the lower layer API GDBusConnection
directly. It replaces the generated code, GDBusInterfaceSkeleton, and
GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
and static descriptor instances of type GDBusInterfaceInfo.
This adds a net plus of more then 1300 lines of hand written code. I claim
that this implementation is easier to understand. Note that previously we
also required extensive and complex glue code to bind our objects to the
generated skeleton objects. Instead, now glue our objects directly to
GDBusConnection. The result is more immediate and gets rid of layers of
code in between.
Now that the D-Bus glue us more under our control, we can address issus and
bottlenecks better, instead of adding code to bend the generated skeletons
to our needs.
Note that the current implementation now only supports one D-Bus connection.
That was effectively the case already, although there were places (and still are)
where the code pretends it could also support connections from a private socket.
We dropped private socket support mainly because it was unused, untested and
buggy, but also because GDBusObjectManagerServer could not export the same
objects on multiple connections. Now, it would be rather straight forward to
fix that and re-introduce ObjectManager on each private connection. But this
commit doesn't do that yet, and the new code intentionally supports only one
D-Bus connection.
Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
for the moment. It could be easily extended later, for example with polling whether the
system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
supported either -- just like before.
Note how NMDBusManager now implements the ObjectManager D-Bus interface
directly.
Also, this fixes race issues in the server, by no longer delaying
PropertiesChanged signals. NMExportedObject would collect changed
properties and send the signal out in idle_emit_properties_changed()
on idle. This messes up the ordering of change events w.r.t. other
signals and events on the bus. Note that not only NMExportedObject
messed up the ordering. Also the generated code would hook into
notify() and process change events in and idle handle, exhibiting the
same ordering issue too.
No longer do that. PropertiesChanged signals will be sent right away
by hooking into dispatch_properties_changed(). This means, changing
a property in quick succession will no longer be combined and is
guaranteed to emit signals for each individual state. Quite possibly
we emit now more PropertiesChanged signals then before.
However, we are now able to group a set of changes by using standard
g_object_freeze_notify()/g_object_thaw_notify(). We probably should
make more use of that.
Also, now that our signals are all handled in the right order, we
might find places where we still emit them in the wrong order. But that
is then due to the order in which our GObjects emit signals, not due
to an ill behavior of the D-Bus glue. Possibly we need to identify
such ordering issues and fix them.
Numbers (for contrib/rpm --without debug on x86_64):
- the patch changes the code size of NetworkManager by
- 2809360 bytes
+ 2537528 bytes (-9.7%)
- Runtime measurements are harder because there is a large variance
during testing. In other words, the numbers are not reproducible.
Currently, the implementation performs no caching of GVariants at all,
but it would be rather simple to add it, if that turns out to be
useful.
Anyway, without strong claim, it seems that the new form tends to
perform slightly better. That would be no surprise.
$ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .; done)
- real 1m39.355s
+ real 1m37.432s
$ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
- real 0m26.843s
+ real 0m25.281s
- Regarding RSS size, just looking at the processes in similar
conditions, doesn't give a large difference. On my system they
consume about 19MB RSS. It seems that the new version has a
slightly smaller RSS size.
- 19356 RSS
+ 18660 RSS
We also do this for libnm, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
GHashTable optimizes a NULL equality function to use direct pointer
comparison. That saves the overhead of calling g_direct_equal().
This is also documented behavior for g_hash_table_new().
While at it, also don't pass g_direct_hash() but use the default
of %NULL. The behavior is the same, but consistently don't use
g_direct_hash().
Instead of having 3 properties @gateway, @never_default and @has_gateway
on NMIP4Config/NMIP6Config that determine the default-route, track the
default-route as a regular route.
The gateway setting is the configuration knob for the default-route.
Since an NMIP4Config/NMIP6Config instance only has one gateway property,
it cannot track more then one default-routes (see related bug rh#1445417).
Especially with policy routing, it might be interesting to configure a
default-route in multiple tables.
Also, later it might be interesting to allow adding default-routes as
regular static routes in a connection, so that the user can configure additional
route parameters for the default-route or add default-routes in multiple tables.
With this patch, default-routes now have a rt_source property according to their
origin.
Also, the previous commits of this branch broke handling of the
default-route :) . That should be working now again.
Default-routes are for the most part like regular routes. Add support to
track them like regular routes in NMIP4Config/NMIP6Config.
One thing is, sometimes we need to figure out whether an ip-config
instance has a default-route. For that, keep track of the best
default-route (there might be multiple) and expose it. That is
the most complicated part of this patch, because there are so many
places where the list of routes gets modified (replace, intersect,
subtract, merge, add), and they all need to take care of updating
the best default-route.
In a next patch, NMDefaultRouteManager will be dropped and default-routes
will be tracked by NMIP4Config/NMIP6Config.
Reasons:
- it adds an O(1) lookup index for accessing NMIPxConfig's addresses.
Hence, operations like merge/intersect have now runtime O(n) instead
of O(n^2).
Arguably, we expect low numbers of addresses in general. For low
numbers, the O(n^2) doesn't matter and quite likely in those cases
the previous implementation was just fine -- maybe even faster.
But the simple case works fine either way. It's important to scale
well in the exceptional case.
- the tracked objects can be shared between the various NMPI4Config,
NMIP6Config instances with NMPlatform and everybody else.
- the NMPObject can be treated generically, meaning it enables code to
handle both IPv4 and IPv6, or addresses and routes. See for example
_nm_ip_config_add_obj().
- I want core to evolve to somewhere where we don't keep copies of
NMPlatformIP4Address, et al. instances. Instead they shall all be
shared. I hope this will reduce memory consumption (although tracking a
reference consumes some memory too). Also, it shortcuts nmp_object_equal()
when comparing the same object. Calling nmp_object_equal() on the
identical objects would be a common case after the hash function
pre-evaluates equality.
Eventually, every NMPlatformIP4Route, NMPlatformIP6Route,
NMPlatformIP4Address and NMPlatformIP6Address should be shared
an deduplicated via the global NMDedupMultiIndex instance.
As first proof of concept, refactor NMIP4Config to track
IPv4 routes via the shared multi_idx. There is later potential
for improvement, when we pass (deduplicated) NMPObject instances
around instead of plain NMPlatformIP4Route, which needs still
a lot of comparing and cloning.
This moves tracking of connectivity to NMDevice and makes the NMManager
negotiate the best of known connectivity states of devices. The NMConnectivity
singleton handles its own configuration and scheduling of the permission
checks, but otherwise greatly simplifies it.
This will be useful to determine correct metrics for multiple default routes
depending on actual internet connectivity.
The per-device connection checks is not yet exposed on the D-Bus, since they
probably should be per-address-family as well.
We need a distinction between external activations and assuming
connections. The former shall have the meaning of devices that are
*not* managed by NetworkManager, the latter are configurations that
are gracefully taken over after restart (but fully managed).
Express that in the activation-type of the active connection.
Also, no longer use the settings NM_SETTINGS_CONNECTION_FLAGS_VOLATILE
flag to determine whether an assumed connection is "external". These
concepts are entirely orthogonal (although in pratice, external
activations are in-memory and flagged as volatile, but the inverse
is not necessarily true).
Also change match_connection_filter() to consider all connections.
Later, we only call nm_utils_match_connection() for the connection
we want to assume -- which will be a regular settings connection,
not a generated one.
The concept of assumed-connection will change. Currently we mark
connections that are generated and assumed as "nm-generated-assumed".
That has several consequences, one of them being that such a settings
connection gets deleted when the device disconnects.
That is, such a settings connection lingers around as long as it's active,
but once it deactivates it gets automatically deleted. As such, it's
a more volatile concept of an in-memory connection.
The concept of such automatically cleaned up connections is useful beyond
generated-assumed. See the related bug rh#1401515.
Currently, we determine NMD_CONNECTION_PROPS_EXTERNAL based
on the settings connection. That is not optimal, because whether
a connection is assumed or externally managed, should be really a
property of the active-connection. So, in the this will change soon
and we would need yet another argument to nm_dispatcher_call().
Instead, drop the settings-connection and applied-connection
arguments and fetch them from the device as needed (but allow
to pass a specific act-request argument to explicitly state
which active connection to use).
Also, rename nm_dispatcher_call() to nm_dispatcher_call_device(),
it this is not a generic dispatcher call, but it is particularly
related to device events. Likewise, rename nm_dispatcher_call_sync()
to nm_dispatcher_call_device_sync().
Remove the redundant action argument. It is clear, that
nm_dispatcher_call_connectivity() is called with action
NM_DISPATCHER_ACTION_CONNECTIVITY_CHANGE.
On the other hand, add the async callbacks. Altough they are
not used at the moment, it seems more correct that an async
API has a callback and a call-id to cancel the invocation.
Keep the include paths clean and separate. We use directories to group source
files together. That makes sense (I guess), but then we should use this
grouping also when including files. Thus require to #include files with their
path relative to "src/".
Also, we build various artifacts from the "src/" tree. Instead of having
individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
unified. Previously, the CFLAGS for each artifact differ and are inconsistent
in which paths they add to the search path. Fix the inconsistency by just
don't add the paths at all.
Unnecessary APIs have been removed from nm-setting-proxy, client like
nm-connection-editor are expected to create a PAC script snippet the load
the location of file in NM.
The API of NMProxyConfig exposes @proxies and @excludes as strv values.
There is no need to track those values internally as a GPtrArray and
then clone them in the getters (especially, since the entire NMProxyConfig
API is internal to core.
Thereby, fix a few memory leaks in add_proxy_config() and some
style fixes for { }.
src: Fixes in nm-device.c and nm-vpn-connection.c to update PacRunner
at the right place and moment. When a device goes up PacRunner is
configured with the Device IPxConfigs and Proxy Config. When it goes
down the same configuration is removed from PacRunner.
ifcfg-rh: Fixed to read and write proxy settings to the ifcfg network
scripts.
In order to pass the connectivity state to the relevant hooks along with
the event itself, we need to add this parameter for the 'Action' method
of then internal 'org.freedesktop.nm_dispatcher' interface, which will
be sent by the network manager main process to the dispatcher.
https://bugzilla.gnome.org/show_bug.cgi?id=768969
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
