The documentation for the ipv4.dhcp-client-id property says:
If the property is not a hex string it is considered as a
non-hardware-address client ID and the 'type' field is set to 0.
However, currently we set the client-id without the leading zero byte
in the dhclient configuration and thus dhclient sends the first string
character as type and the remainder as client-id content. Looking
through git history, the dhclient plugin has always behaved this way
even if the intent was clearly that string client-id had to be zero
padded (this is evident by looking at
nm_dhcp_utils_client_id_string_to_bytes()). The internal plugin
instead sends the correct client-id with zero type.
Change the dhclient plugin to honor the documented behavior and add
the leading zero byte when the client-id is a string.
This commit introduces a change in behavior for users that have
dhcp=dhclient and have a plain string (not hexadecimal) set in
ipv4.dhcp-client-id, as NM will send a different client-id possibly
changing the IP address returned by the server.
https://bugzilla.gnome.org/show_bug.cgi?id=793957
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 next commit will completely rework NMBusManager and replace
NMExportedObject by a new type NMDBusObject.
Originally, NMDBusObject was added along NMExportedObject to ease
the rework and have compilable, intermediate stages of refactoring. Now,
I think the new name is better, because NMDBusObject is very strongly related
to the bus manager and the old name NMExportedObject didn't make that
clear.
I also slighly prefer the name NMDBusObject over NMBusObject, hence
for consistancy, also rename NMBusManager to NMDBusManager.
This commit only renames the file for a nicer diff in the next commit.
It does not actually update the type name in sources. That will be done
later.
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.
The generated code is really just a thin wrapper around direct
GDBusProxy calls. GDBusProxy is reasonably convenient to use directly,
drop this wrapper.
We also don't use a generated wrapper for other cases where
NetworkManager acts as a D-Bus client. There is no reason to
do it in this case.
While the nmdbus_*() functions that we were using are small wrappers,
we also created a NMDBusSecretAgent instance, and hence several other
functions and symbols are used as well. It's unnecessary.
This saves 8552 bytes for NetworkManager binary (2817944 vs. 2809392
bytes for contrib/rpm on x86_64).
On exit during NMManager's dispose(), we must fist remove active connections
via active_connection_remove(), before clearing the volatile-connection-list.
Otheriwise, while deleting the active connection, we schedule a idle action
to delete the volatile connection on idle, but at that point the dispose()
already cleaned up the idle list.
==3150== 72 (24 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 3,411 of 6,079
==3150== at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
==3150== by 0x6AB7358: g_malloc (gmem.c:94)
==3150== by 0x6ACEF35: g_slice_alloc (gslice.c:1025)
==3150== by 0x1686B1: connection_flags_changed (nm-manager.c:1823)
==3150== by 0x661F73C: g_closure_invoke (gclosure.c:804)
==3150== by 0x66324DD: signal_emit_unlocked_R (gsignal.c:3635)
==3150== by 0x663AD04: g_signal_emit_valist (gsignal.c:3391)
==3150== by 0x663B66E: g_signal_emit (gsignal.c:3447)
==3150== by 0x2EC753: connection_flags_changed (nm-settings.c:824)
==3150== by 0x661F73C: g_closure_invoke (gclosure.c:804)
==3150== by 0x66324DD: signal_emit_unlocked_R (gsignal.c:3635)
==3150== by 0x663AD04: g_signal_emit_valist (gsignal.c:3391)
==3150== by 0x663B66E: g_signal_emit (gsignal.c:3447)
==3150== by 0x6623C03: g_object_dispatch_properties_changed (gobject.c:1080)
==3150== by 0x1DFD47: dispatch_properties_changed (nm-dbus-object.c:237)
==3150== by 0x6626178: g_object_notify_by_spec_internal (gobject.c:1173)
==3150== by 0x6626178: g_object_notify_by_pspec (gobject.c:1283)
==3150== by 0x2E7377: _notify (nm-settings-connection.c:53)
==3150== by 0x2E7377: nm_settings_connection_set_flags_full (nm-settings-connection.c:2346)
==3150== by 0x2E744D: nm_settings_connection_set_flags (nm-settings-connection.c:2316)
==3150== by 0x2E7466: set_visible (nm-settings-connection.c:316)
==3150== by 0x2E7774: nm_settings_connection_delete (nm-settings-connection.c:795)
==3150== by 0x1665A8: _delete_volatile_connection_do (nm-manager.c:598)
==3150== by 0x1668F4: active_connection_remove (nm-manager.c:625)
==3150== by 0x16ABA7: dispose (nm-manager.c:6726)
==3150== by 0x6624607: g_object_unref (gobject.c:3293)
==3150== by 0x1D779B: _nm_singleton_instance_destroy (nm-core-utils.c:138)
==3150== by 0x4011332: _dl_fini (in /usr/lib64/ld-2.26.so)
==3150== by 0x815FB57: __run_exit_handlers (in /usr/lib64/libc-2.26.so)
==3150== by 0x815FBA9: exit (in /usr/lib64/libc-2.26.so)
==3150== by 0x1383C7: main (main.c:467)
NMTST_ASSERT_PLATFORM_NETNS_CURRENT() already checks that the current namespace
is correct. Remove the duplicate assertion.
Also, NMP_CACHE_OPS_UNCHANGED is numerically identical to NM_PLATFORM_SIGNAL_NONE.
Use it in the assertion.
Return the extended ack message from the WaitForNlResponse delayed
action so that the caller can print a detailed reason with the
appropriate logging level.
From v4.12 the kernel appends some attributes to netlink acks
containing a textual description of the error and other fields (see
commit [1]). Parse those attributes and print the error message.
Examples:
platform-linux: netlink: recvmsg: error message from kernel: Network is unreachable (101) "Nexthop has invalid gateway" for request 12
platform-linux: netlink: recvmsg: error message from kernel: Invalid argument (22) "Local address cannot be multicast" for request 21
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d4bc93368f5a0ddb57c8c885cdad9c9b7a10ed5
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
The condition was obviosly inverted, blocking autoconnect when
it should not, and not blocking it when it should.
[thaller@redhat.com: modified original patch and rewrite commit message]
Fixes: e2c8ef45achttps://bugzilla.gnome.org/show_bug.cgi?id=794014
IPv4 routes that are a response to RTM_GETROUTE must have the cloned
flag while IPv6 routes don't have to. Don't check the flag for IPv6
routes and add a test case to verify that RTM_GETROUTE works for IPv6.
https://bugzilla.gnome.org/show_bug.cgi?id=793962
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.
Always read and load ipv4 property values when method is shared also if
they will not be used: instead of dropping them at connection update,
keep their values in the ifcfg file.
Exceptions: ipv4.dns and ipv4.dns-search. They will be not read, otherwise
they may trigger a failure in nm-setting-ip4-config.c:verify() on load.
https://bugzilla.redhat.com/show_bug.cgi?id=1519299
Passing ownership with a function call is confusing. Don't do that.
Since we have the cleanup attribute, it doesn't significantly
complicate the callers, as all they need to do is marking the
@msg variable to free the message when going out of scope.
That results in the function that allocates the message also being
responsible to free it.
The callers expect _nl80211_send_and_recv() to free @msg.
This was broken by the previous commit, which wrongly removed
the nm_auto_nlmsg cleanup attribute.
Fix the compiler warning differently.
Fixes: d7108d9362
systemd commit f11cba7479fe ("libsystemd-network: fix unaligned loads
(issue #7654)") changed the way in which the MAC address is read to
use native endiannes:
htobe32(*((uint32_t *)x) -> unaligned_read_ne32(x)
This is wrong because loads done with BPF_LD + BPF_ABS are big-endian, as it
can be seen for the ethertype and arp-operation loads above in the
filter. Also, the same commit changed:
htobe32(*((unsigned int *)x) -> unaligned_read_be32(x)
in _bind_raw_socket(), which is the correct form.
The commit broke IPv4LL in presence of loops, as the sender now considers its
own packets as conflicting.
systemd commit: 6afe9046d570fd86dbc04526b5685c64d7958086
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.
- don't even bother to look into the platform cache, but use
if_indextoname() / if_nametoindex(). In most cases, we obtained
the ifindex/ifname not from the platform cache in the first
place. Hence, there is a race, where the interface might not
exist.
However, try to process events of the platform cache, hoping
that the cache contains an interface for the given ifindex/ifname.
- let set_ip_ifindex() and set_ip_iface() both return a boolean
value to indicate whether a ip-interface is set or not. That is,
whether we have a positive ip_ifindex. That seems more interesting
information, then to return whether anything changed.
- as before, set_ip_ifindex() can only clear an ifindex/ifname,
or error out without doing anything. That is different from
set_ip_iface(), which will also set an ifname if no ifindex
can be resolved. That is curreently ugly, because then ip-ifindex
and ip-iface don't agree. That shall be improved in the future
by:
- trying to set an interface that cannot be resolved shall
lead to a disconnect in any case.
- we shall make less use of the ip-iface and rely more on the
ifindex.
