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.
When the ovs interface gets deactivated, it is released from the
master port and we call nm_device_update_from_platform_link (dev,
NULL) to ignore any later event for the interface. This is important
especially because it sets a zero ifindex on the interface and so,
later when the link disappears, we don't unmanage the device but
directly remove it.
However, since ovs commands are queued, the link could appear during
the deactivation and we need to ignore such events. Add a new device
method can_update_from_platform_link() for such purpose.
Tracking the deletion of link by ifindex is difficult because the
ifindex of the device is updated through delayed (idle) calls in
NMDevice and so there is the possibility that at a certain time the
device ifindex is not in sync with platform state. It seems simpler to
watch instead the interface name. The ugly thing is that the interface
name can be changed externally, but if users do that on an activating
device they are looking for trouble.
Also change the deactivate code to deal with the scenario where we
already created the interface in the ovsdb but the link didn't show up
yet. To ensure a proper cleanup we must wait that the link appears and
then goes away; however the link may never appear if vswitchd sees
only the last state in ovsdb, and so we must use a ugly timeout to
avoid waiting forever.
https://bugzilla.redhat.com/show_bug.cgi?id=1787989
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
At this point, just move the fields in their respective address-family
specific structure. We don't use it generically yet, but instead always
explicitly select IPv4 or IPv6. But this would allow to access those
fields by address-family in the future.
Often, the code paths for IPv4 and IPv6 are very similar. We should try
to unify those code paths. The main advantage of doing that, is that
we don't unintentionally end up doing different things. And of course,
it removes duplicate code.
In a first step, unify handling of the grace timeout for DHCPv4 and
DHCPv6.
We will add a property NM_NDISC_RA_TIMEOUT for which this name is better
suited. The problem is really that our convention for object properties
and signals defines have no prefix to indicate whether it's a property
or a signal.
Rename.
It is undefined behavior and can lead to crashes or memory corruption.
In practice, this only had an issue on Big Endian systems.
Fixes: fdbf4ae5e6 ('ifcfg-rh: add IPV4_DHCP_TIMEOUT key for ipv4.dhcp-timeout property')
If a function is only called once, it may not help to simplify the code
but make it more complicated. It would only simplify the code, if it
had a clear, distinct purpose. That isn't the case here. Also, the
IPv4 writer doesn't have such a function either. Drop and inline it.
This is C, we have almost no IDE support. And ctags/cscope is much more
helpful if we use unique names.
Don't use the get_dhcp_timeout() name, because that is already used in
"src/devices/nm-device.c" already. Rename.
