These fields have the same purpose for IPv4 and IPv6. Also, they have an alias
with name _x, that can be indexed by an IS_IPv4 1/0 value.
Rename the fields so that the distinguisher 4/6/x is at the end. The point
is to make the name more similar.
Functions like these are conceptually very similar. Commonly,
what we want to do for one address family we also want to do
for the other.
Merge the two functions. This moves the similar parts closer
to each other and stand beside it. This is only the first part
of the merge, which is pretty trivial without larger changes
(to keep the diff simple). More next.
Now that there is no difference between initial capturing of
the configuration, and a later update_ip_config() call during
a signal from platform, we only need to make sure that the
IP config instances are initialized at least once.
In case we are called multiple times, there is nothing to do.
This was called by via
...
- manager:recheck_assume_connection()
- manager:get_existing_connection()
- nm_device_capture_initial_config()
- update_ext_ip_config(initial=TRUE)
and would parse resolv.conf, and try to fill the device IP config
with nameservers and dns-options.
But why? It would only have effect if NM was started with
nm_dns_manager_get_resolv_conf_explicit(), but is that really sensible?
And it would only take effect on devices that have a default route.
And for what is this information even used?
Let's not do it this way. If we need this information for assuming or
external sys-iface mode, then it should be explicitly loaded at the
appropriate moment.
For now, drop it and see what breaks. Then we can fix it properly. If
it even matters.
Drop capture_lease_config(). It was added by commit
0321073b3c.
Note that it was only called by
...
- manager:recheck_assume_connection()
- manager:get_existing_connection()
- nm_device_capture_initial_config()
- update_ext_ip_config(addr_family=AF_INET, initial=TRUE)
- capture_lease_config()
It had only effect when the device had IPv4 permanent addresses.
But then, capture_lease_config() would go on and iterate over
all connections (sorted by last-connect timestamp). It would
consider connection candidates that are compatible with the device,
and try to read the lease information from disk
It's really unclear what this means. For assuming (graceful take over),
do we need the lease information in the device? I don't think so,
because we will match an existing connection. The lease information
shall be read while activating (if necessary).
For external connections, we don't even have a matching connection
and we always generate a new one. It doesn't seem right to consider
leases from disk, for a different connection.
Just drop this. It's really ugly. If this causes an issue, it must be
fixed differently. We want to behave determinstically and well defined.
I don't even comprehend all the implications of what this had.
Also note that update_ext_ip_config() no longer clears
priv->dev_ip4_config.
A failure to configure an address family does not mean that the connection
is going to fail. It depends, for example on ipvx.may-fail.
Always export the NMIPxConfig instance in that case.
linklocal6_complete() had only one caller. The caller would check
whether the conditions for linklocal6_complete() are satisfied, and
then call it. Note that linklocal6_complete() would again assert
that these conditions hold. Don't do this. Just move the check
inside linklocal6_complete(), and rename to linklocal6_check_complete().
Also, linklocal6_complete() was called by update_ip_config(),
which was called by nm_device_capture_initial_config() and
queued_ip6_config_change().
It doesn't make sense to call linklocal6_complete() during
nm_device_capture_initial_config(). Move the call to
queued_ip6_config_change().
Likewise, in ndisc_ra_timeout() we also want to include tentative
addresses. Looking into priv->ip6_config to determine whether
an other IP configuration is active, is anyway odd, and likely
a bug.
Instead have one particular nm_ip6_config_get_address_first_nontentative() function,
make it more extendable. Now, we pass a match-type argument, which can control which
element to search.
This patch has no change in behavior, but it already makes clear, that
nm_ip6_config_get_address_first_nontentative() was buggy, because it would
also return addresses that failed DAD.
Don't do "if (var == FALSE)" for boolean variables.
Also, make booleans in NMDevicePrivate structure bitfields
and reorder the fields beside other bitfields. This allows
a tighter packing of the structure.
addrconf6_start_with_link_ready() cannot fail. Hence, don' return a boolean
success value.
linklocal6_start() can only either POSTPONE or succeed right away. Don't return
a NMActStageReturn value, TRUE/FALSE is enough.
This simplifies the callers that don't have to check for values that never
come.
NMSettings exposes a cached list of all connection. We don't need
to clone it. Note that this is not save against concurrent modification,
meaning, add/remove of connections in NMSettings will invalidate the
list.
However, it wasn't save against that previously either, because
altough we cloned the container (GSList), we didn't take an additional
reference to the elements.
This is purely a performance optimization, we don't need to clone the
list. Also, since the original list is of type "NMConnection *const*",
use that type insistently, instead of dependent API requiring GSList.
IMO, GSList is anyway not a very nice API for many use cases because
it requires an additional slice allocation for each element. It's
slower, and often less convenient to use.
nm_modem_complete_connection() cannot just return FALSE in case
the modem doesn't overwrite complete_connection(). It must set
the error variable.
This leads to a crash when calling AddAndActivate for Ofono type
modem. It does not affect the ModemManager implementation
NMModemBroadband, because that one implements the method.
Kernel recently got support for exposing TUN/TAP information on netlink
[1], [2], [3]. Add support for it to the platform cache.
The advantage of using netlink is that querying sysctl bypasses the
order of events of the netlink socket. It is out of sync and racy. For
example, platform cache might still think that a tun device exists, but
a subsequent lookup at sysfs might fail because the device was deleted
in the meantime. Another point is, that we don't get change
notifications via sysctl and that it requires various extra syscalls
to read the device information. If the tun information is present on
netlink, put it into the cache. This bypasses checking sysctl while
we keep looking at sysctl for backward compatibility until we require
support from kernel.
Notes:
- we had two link types NM_LINK_TYPE_TAP and NM_LINK_TYPE_TUN. This
deviates from the model of how kernel treats TUN/TAP devices, which
makes it more complicated. The link type of a NMPlatformLink instance
should match what kernel thinks about the device. Point in case,
when parsing RTM_NETLINK messages, we very early need to determine
the link type (_linktype_get_type()). However, to determine the
type of a TUN/TAP at that point, we need to look into nested
netlink attributes which in turn depend on the type (IFLA_INFO_KIND
and IFLA_INFO_DATA), or even worse, we would need to look into
sysctl for older kernel vesions. Now, the TUN/TAP type is a property
of the link type NM_LINK_TYPE_TUN, instead of determining two
different link types.
- various parts of the API (both kernel's sysctl vs. netlink) and
NMDeviceTun vs. NMSettingTun disagree whether the PI is positive
(NM_SETTING_TUN_PI, IFLA_TUN_PI, NMPlatformLnkTun.pi) or inverted
(NM_DEVICE_TUN_NO_PI, IFF_NO_PI). There is no consistent way,
but prefer the positive form for internal API at NMPlatformLnkTun.pi.
- previously NMDeviceTun.mode could not change after initializing
the object. Allow for that to happen, because forcing some properties
that are reported by kernel to not change is wrong, in case they
might change. Of course, in practice kernel doesn't allow the device
to ever change its type, but the type property of the NMDeviceTun
should not make that assumption, because, if it actually changes, what
would it mean?
- note that as of now, new netlink API is not yet merged to mainline Linus
tree. Shortcut _parse_lnk_tun() to not accidentally use unstable API
for now.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1277457
[2] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=1ec010e705934c8acbe7dbf31afc81e60e3d828b
[3] https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=118eda77d6602616bc523a17ee45171e879d1818https://bugzilla.redhat.com/show_bug.cgi?id=1547213https://github.com/NetworkManager/NetworkManager/pull/77
With g_clear_pointer(pptr, g_free), pptr is cast to a non-const pointer,
and hence there is no compiler warning about calling g_free() in a const
pointer. However, it still feels ugly to pass a const pointer to
g_clear_pointer(). We should either add an explicity cast, or just
make the pointer non-const.
I guess part of the problem is that C's "const char *" means that the
string itself is immutable, but also that it cannot be freed. We most
often want a different semantic: the string itself is immutable after
being initialized once, but the memory itself can and will be freed.
Such a notion of immutable C strings cannot be expressed.
For that, just remove the "const" from the declarations. Although we
don't want to modify the (content of the) string, it's still more a
mutable string.
Only in _vt_cmd_obj_copy_lnk_vlan(), add an explicity cast but keep the
type as const. The reason is, that we really want that NMPObject
instances are immutable (in the sense that they don't modify while
existing), but that doesn't mean the memory cannot be freed.
The g_clear_pointer() macro already contains a cast to GDestroyNotify. No
need to do it ourself. In fact, with the cast, this only works with the
particular g_clear_pointer() implementation, that first assigns the
destroy function to a local variable.
See-also: https://bugzilla.gnome.org/show_bug.cgi?id=674634#c52
The test tries to do IPv4 DAD. That necessarily involves waiting
for a timeout. Since the NMArpingManager spawns arping processes,
the precise timings depend on the load of the machine and may be
large in some cases.
Usually, our test would run fast to successful completion.
However, sometimes, it can take several hundered milliseconds.
Instead of increasing the timeout to a large value (which would
needlessly extend the run time of our tests in the common cases),
try first with a reasonably short timeout. A timeout which commonly
results in success. If the test with the short timeout fails, just
try again with an excessively large timeout.
This saves about 400 msec for the common case, but extends the
races that we saw where not even 250 msec of wait time were
sufficient.
Previously we would kill the client when the lease expired and we
restarted it 3 times at 2 minutes intervals before failing the
connection. If the client is killed after it received a NACK from the
server, it doesn't have the chance to delete the lease file and the
next time it is started it will request the same lease again.
Also, the previous restart logic is a bit convoluted.
Since clients already know how to deal with NACKs, let them continue
for a grace period after the expiry. When the grace period ends, we
fail the method and this can either fail the whole connection or keep
it active depending on the may-fail configuration.
https://bugzilla.gnome.org/show_bug.cgi?id=783391
I dislike the static hash table to cache the integer counter for
numbered paths. Let's instead cache the counter at the class instance
itself -- since the class contains the information how the export
path should be exported.
However, we cannot use a plain integer field inside the class structure,
because the class is copied between derived classes. For example,
NMDeviceEthernet and NMDeviceBridge both get a copy of the NMDeviceClass
instance. Hence, the class doesn't contain the counter directly, but
a pointer to one counter that can be shared between sibling classes.
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
The change doesn't really make a difference. I thought it would, so I
did it. But turns out (as the code correctly assumes), while the
notifications are frozen, it's OK to leave the property still in an
inconsistent state while emitting the notify signal.
Still, it feels slightly more correct this way, so keep the change.
The notify() signal is not emitted while the object properties are
blocked via g_object_freeze_notify(). That makes is unsuitable to
emit a notification for "peer" property whenver the device's "parent"
property changes. Because especially with freeze/thaw, we want to emit
both signals in the same batch, not first emit change signals for "parent",
and then in a second run the signals for "peer".
Use the existing parent_changed_notify() virtual function instead.
NAP connections are a bit special, in that they also have a [bridge]
setting, but their connection.type is "bluetooth".
The canonical way to check whether a bluetooth connection is of NAP type
is by calling _nm_connection_get_setting_bluetooth_for_nap().
So, instead of checking for bluetooth.type "pan" or "dun", check the
opposite and whether the connection is of NAP type. In practice it's the
same, but let'check for NAP consistently via get_setting_bluetooth_for_nap().
Bluetooth tethering using DUN or PANU is a common way to expose a
metered 3G or 4G connection from a phone to a laptop. We deliberately
ignore NAP connections, which is where we’re sharing internet from the
laptop to another device.
We could also set GUESS_YES for WiMAX connections, but NetworkManager
doesn’t support them any more. Add a comment about that.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=794120
If SSID is an empty string there's no need to call nm_wifi_ap_set_ssid
as it won't do anything. It also has an assert checking that NULL is
passed for an empty SSID and we were passing a non-NULL pointer.
This bug was not causing a crash for me because of the !IS_NM_DEVICE_IWD
check and because my glib version probably had the assertion within
NM_IWD_MANAGER_GET_PRIVATE disabled.
While there, change the g_signal_connect line to use the macro for the
signal name.
Make sure .set_enabled uses the Device.Powered property to basically
bring the netdev UP and DOWN as I understand is expected by the
nm_device logic.
Device.Powered should generally reflect the UP state immediately but
just to avoid possible race conditions .is_available() will now return
a value that is an AND of the local "enabled" state and IWD's Powered
property.
Change from the default dbus call timeout (-1) to infinite (G_MAXINT)
because the call may now include the secret requests which have their
own timeout policies.
Remove the code (mostly copied from nm-device-wifi.c) that handles
checking if the secrets were provided and requesting missing secrets
before starting a connection attempt. Instead, request secrets when
we're asked for them by IWD through its agent interface. This happens
while the dbus Connect call is running. We change the NMDevice from
the CONFIG state to NEED_AUTH and then change back to CONFIG once we
sent the secrets back to IWD.
The current code would require the secrets only based on whether a
network is a KnownNetwork but IWD may need a new passwords even for
KnownNetworks if the last connection attempt has failed.
Now that every call to nm_device_set_ip_iface() and nm_device_set_ip_ifindex()
is checked, and setting an interface that does not exist causes the device
state to fail, we no longer need to allow setting an ip-iface if we are
unable to retrieve the ip-ifindex.
Depending on the bearer's configuration method, the data-port is
either a networking interface, or an tty for ppp.
Let's treat them strictily separate.
Also, rework how NM_MODEM_DATA_PORT was used in both contexts.
Instead, use the that we actually care about.
Also, when nm_device_set_ip_ifindex() fails, fail activation
right away.
Also, we early try to resolve the network interface's name to
an ifindex. If that fails, the device is already gone and we
fail early.
nm_device_modem_new() is only called with a newly created
NMModemBroadband or NMModemOfono instance.
See the callers
- NMModemManager:handle_new_modem()
- NMWwanFactory:modem_added_cb()
- NMDeviceModem:nm_device_modem_new()
Hence, at that point, the modem cannot yet have a data-port
or ip-iface set, because that is only obtained later.
