Configuration stages like act_stage2_config() can postpone progressing
to the next stage. Currently, when the condition that we wait for gets
satisfied, the code schedules the next stage from there.
I think that is wrong, because when we postpone from act_stage2_config(),
follow up steps of stage2 get skipped. Thus, when we are ready to progress,
the class should enter stage 2 again.
This requires that stage2 becomes reentrant and that the code reenters the
same stage.
We usually want to schedule stage2 when we just completed with the previous
stage (or, if we are currently in stage2, and want to re-enter it).
In those cases, the conditions are often right to just proceed right away.
No need to schedule the stage on an idle handler. Allow to invoke stage2
right away.
There was only API to schedule the stage on an idle handler.
Sometimes, we are just in the right situation to schedule the stage
right away. It should be possibly to avoid going through the extra hop.
For now, none of the caller makes use of this. So, there isn't any
actual change in behavior. But by adding this possibility, we may do
use in the future.
Avoid GDBusProxy, instead use GDBusConnection directly. I very much
prefer this because that way we have explicit control over what happens
on D-Bus. With GDBusProxy this is hidden under another layer of complex
code. The hardest part when using a D-Bus interface is to manage the
state via an asynchronous medium. GDBusProxy contains state about the
D-Bus interface and duplicate the state that we track. This makes it hard
to reason about things.
Rework creation of NMSupplicantInterface. Previously, a NMSupplicantInterface
had multiple initialization states. In particular, the first state would not
yet tie the interface to a certain D-Bus object path. Instead, NMSupplicantInterface
would try and retry to create the D-Bus object.
Now, NMSupplicantManager has an asynchronous method to create interface
instances. The manager only creates an interface instance after the D-Bus
path is known. That means, a NMSupplicantInterface instance is now
strongly tied to a name-owner and D-Bus path.
It follows that the state of NMSupplicantInterface can only go from STARTING,
via the supplicant states, to DOWN. Never back. That was already previously
the case that the state from DOWN was final and once the 3 initial
states were passed, the interface's state would never go back to the initial
state. Now this is more strict and more formalized. The 3 initialization states
are combined.
I think the tighter state handling simplifies users of NMSupplicantInterface.
See for example "nm-device-ethernet.c". It's still complicated, because handling
state is fundamentally difficult.
NMSupplicantManager will take care to D-Bus activate wpa_supplicant only
when necessary (poke). Previously, creating the manager instance
would always start suppliant service. Now, it's started on demand.
Add '_nm_setting_bond_get_option_or_default()' and move all the custom
policies applied by NM for bond options in there.
One such example of a custom policy is to set 'miimon' to 0 (instead of its
default value of 100) if 'arp_interval' is explicitly enabled
and 'miimon' is not.
This means removing every piece of logic from
nm_setting_bond_add_option() which used to clear out 'arp_interval' and
'arp_ip_target' if 'miimon' was set or clear out 'miimon' along with
'downdelay', 'updelay' and 'miimon' if 'arp_interval' was set.
This behaviour is a bug since the kernel allow setting any combination
of this options for bonds and NetworkManager should not limit the user
to do so.
Also use 'set_bond_attr_or_default()' instead of 'set_bond_attr()' as
the former calls '_nm_setting_bond_get_option_or_default()' to implement
the right logic to retrieve bond options according to current bond
configuration.
Don't do a linear search through all names, but use binary search.
Upside: calling nms_ifcfg_rh_utils_get_ethtool_by_name() in a loop
(once over all 60 names) is 75% faster.
Downside: when adding a new feature, we have yet another line that we
need to add. Previously, adding a new feature required adding 7 lines,
not it is 8. But we didn't add a single feature since this was added,
so that happens very seldom.
Possible downside: is this code harder to read? Now we track both how to
convert the ID to name and back. This is redundant (and thus harder to
maintain). But it's really just one extra line per feature, for which there
is a unit test. So, when adding a new NMEthtoolID it would be pretty
hard to mess this up, because of all the tests and assertions.
So, maybe it's slightly harder to read. On the other hand, it unifies
handling for ethtool and kernel names, and the code has less logic
and is more descriptive. I don't think this is actually harder to maintain
and it should be easy to see that it is correct (readability).
Otherwise, we only prune unused files when the service terminates.
Usually, NetworkManager service doesn't get restarted before shutdown
of the system (nor should it be). That means, if you create (and
destroy) a large number of software devices, the state files pile
up.
From time to time, go through the files on disk and delete those that
are no longer relevant.
In this case, "from time to time" means after we write/update state
files 100 times.
nm_manager_write_device_state() writes the device state to a file. The ifindex
is here important, because that is the identifier for the device and is also
used as file name. Return the ifindex that was used, instead of letting the
caller reimplement the knowledge which ifindex was used.
It was not very clear whether update_dns() would always set the error
output variable if (and only if) it would return false.
Rework the code to make it clearer.
Not using cleanup attribute is error prone.
In theory, a function should only return a GError if (and only if) it
signals a failure. However, for example in commit 324f67956a ('dns:
ensure to log a warning when writing /etc/resolv.conf fails') due to
a bug we was violated. In that case, it resulted in a leak.
Avoid explicit frees and use the gs_free_error cleanup attribute
instead. That would also work correctly in face of such a bug and in
general it seems preferable to explicitly assign ownership to auto
variables on the stack.
When setting "main.rc-manager=symlink" (the default) and /etc/resolv.conf
is a file, NetworkManager tries to write the file directly. When that fails,
we need to make sure to propagate the error so that we log a warning about that.
With this change:
<debug> [1583320004.3122] dns-mgr: update-dns: updating plugin systemd-resolved
<trace> [1583320004.3123] dns-sd-resolved[f9e3febb7424575d]: send-updates: start 8 requests
<trace> [1583320004.3129] dns-mgr: update-resolv-no-stub: '/var/run/NetworkManager/no-stub-resolv.conf' successfully written
<trace> [1583320004.3130] dns-mgr: update-resolv-conf: write to /etc/resolv.conf failed (rc-manager=symlink, $ERROR_REASON)
<trace> [1583320004.3132] dns-mgr: update-resolv-conf: write internal file /var/run/NetworkManager/resolv.conf succeeded
<trace> [1583320004.3133] dns-mgr: current configuration: [{ [...] }]
<warn> [1583320004.3133] dns-mgr: could not commit DNS changes: $ERROR_REASON
<info> [1583320004.3134] device (eth0): Activation: successful, device activated.
https://bugzilla.redhat.com/show_bug.cgi?id=1809181
The GSource must also be unrefed. Also, first clear the field
before invoking callbacks to the upper layers.
Fixes: 843d696e46 ('dhcp: clean source on dispatch failure')
Fix the following warning:
NetworkManager[1524461]: Source ID 3844 was not found when attempting to remove it
g_logv (log_domain=0x7f2816fa676e "GLib", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7ffe697374d0) at gmessages.c:1391
g_log (log_domain=log_domain@entry=0x7f2816fa676e "GLib", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7f2816fae240 "Source ID %u was not found when attempting to remove it") at gmessages.c:1432
g_source_remove (tag=519) at gmain.c:2352
nm_clear_g_source (id=<optimized out>) at ./shared/nm-glib-aux/nm-macros-internal.h:1198
dispose (object=0x55f7289b1ca0) at src/dhcp/nm-dhcp-nettools.c:1433
g_object_unref (_object=<optimized out>) at gobject.c:3303
g_object_unref (_object=0x55f7289b1ca0) at gobject.c:3232
dhcp4_cleanup (self=self@entry=0x55f728af3b20, cleanup_type=cleanup_type@entry=CLEANUP_TYPE_DECONFIGURE, release=release@entry=0) at src/devices/nm-device.c:7565
...
Fixes: 45521b1b38 ('dhcp: nettools: move to failed state if event dispatch fails')
Maybe the reader should not try to add its own validation. It
could just read the value, set it in the profile, and let
nm_connection_verify() handle it.
However:
- in this form the code only logs a warning about invalid setting.
If we let it come to nm_connection_verify(), the connection profile
will be entirely rejected. I think this makes sense, because ifcfg
files may be edited by the user and we don't know what is out there.
- it's nicer to show a warning that specifically mentions the DEVICE=
variable. There error message we get from nm_connection_verify()
is no longer aware of ifcfg peculiarities.
Instead: use the appropriate validation function.
It's very unlikely that we have actual blobs for a Wi-Fi network.
That is because the settings plugins (keyfile, ifcfg-rh) convert
blobs to files on disk when writing the profile. So, you can only
have them by editing the files directly to contain blobs.
At that point, don't always create the GHashTable for blobs.
Fail the enslavement of the ovs port if the bridge device is not
found, instead of generating assertions and potentially crash later.
https://bugzilla.redhat.com/show_bug.cgi?id=1797696
Fixes: 101e65d2bb ('ovs: allow changing mac address of bridges and interfaces')
The previous code tried to get the bridge active connection and it
used the port active connection instead in case of failure. This
doesn't seem right, as in nm-ovsdb.c the bridge AC is used to get the
bridge settings (including the uuid, interface name, and cloned mac).
In case of failure getting the bridge AC we should just fail.
Fixes: 830a5a14cb ('device: add support for OpenVSwitch devices')
Surisingly, the compiler may detect the remaining obj_type in
the default switch. Then, inlining nmp_class_from_type() it may detect
that this is only possible to hit with an out or range access to
_nmp_classes array.
Rework the code to avoid that compiler warning. It's either way not
supposed to happen.
Also, drop the default switch case and explicitly list the enum values.
Otherwise it is error prone to forget a switch case.
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.
It would be better if we would be able to use NMLinkType enum
as an index (e.g. into an array of LinkDesc structures). For that,
it is necessary that the enum is just consecutive numbers.
Don't assign special meaning to the enum. Also, this was only
used at two places, that we can solve differently.
Internally, the options are tracked in a hash table and of undefined
sort order. However, nm_setting_bond_get_option() always returns a stable
(sorted) order.
Move "mode" as first, because that is usually the most interesting option.
The effect is:
$ nmcli -o connection show "$BOND_PROFILE"
...
-bond.options: arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,mode=balance-rr,use_carrier=0
+bond.options: mode=balance-rr,arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,use_carrier=0
This doesn't affect keyfile, which sorts the hash keys themself (and
doesn't treat the "mode" special).
This however does affect ifcfg-rh writer how it writes the BONDING_OPTS
variable. I think this change is fine and preferable.
When AddConnection() or Update() terminate, the (unrealized) virtual
device should be already be available, otherwise an activation attempt
of that connection can fail.
https://bugzilla.redhat.com/show_bug.cgi?id=1804350
This reverts commit c163207b07.
