NetworkManager/NetworkManager!424

When creating a new connection before the user gets the chance to modify
the bond options let's just initialize 'mode' and 'miimon' (with related
'updelay' and 'downdelay').

Initializing also 'arp_interval', 'arp_ip_target' and 'primary' doesn't
make much sense as by default they're disabled or contain empty values.

Add 'nm_setting_bond_get_option_normalized()', the purpose of this API
is to retrieve a bond option normalized value which is the option that
NetworkManager will actually apply to the bond when activating the
connection, this takes into account default values for some options that
NM assumes.

For example, if you create a connection:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0

Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return "100" as even if not specified NetworkManager enables miimon for
bond connections.

Another example:
$ nmcli c add type bond con-name nm-bond ifname bond0 bond.options mode=0,arp_interval=100

Calling 'nm_setting_bond_get_option_normalized(s_bond, "miimon")' would
return NULL in this case because NetworkManager disables miimon if
'arp_interval' is set explicitly but 'miimon' is not.

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.

Fix 'miimon' and 'arp_interval' validation, they can both be set indeed,
the kernel does not impose this limitation, nevertheless is sensible to
keep the defaults as previously (miimon=100, arp_interval=0).

Also add unit test.

Doing 'verify()' with options such as 'miimon' and 'num_grat_arp' set to
arbitrary values it's not consistent with what NetworkManager does to
bond options when activating the bond through 'apply_bonding_config()'
(at a later stage) because the said values do not
correspond to what the default values for those options are.

This leads to an inconsistency with the 'miimon' parameter for example,
where 'verify()' is done while assuming it's 0 if not set but its
default value is actually 100.

Fixes: 8775c25c ('libnm: verify bond option in defined order')

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).

Auke-Jan Kok authored Mar 05, 2020 and Thomas Haller committed Mar 06, 2020

Add the only important example that this file should have

All other examples are nice. But when you install a console-only
machine and read the NM man pages, you are none the wiser, because
something as simple like this isn't covered in the man pages.

I've seen other users complain about it, and I've torn my hair out
over this several times.

[thaller@redhat.com: changed subject line of patch]

https://bugzilla.redhat.com/show_bug.cgi?id=1810153

NetworkManager/NetworkManager!429

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's just a more fitting name after the previous change.

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 324f6795 ('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: 843d696e ('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: 45521b1b ('dhcp: nettools: move to failed state if event dispatch fails')

The 'Before' dependency between NM-dispatcher and NM causes a deadlock
when stopping the NM service. When terminating, NM wants to D-Bus
activate NM-dispatcher to synchronously handle pre-down events; but
NM-dispatcher start is ordered after NM shutdown due to the following
behavior described in systemd.unit(5) man page:

  Given two units with any ordering dependency between them, if one
  unit is shut down and the other is started up, the shutdown is
  ordered before the start-up. It doesn't matter if the ordering
  dependency is After= or Before=, in this case. It also doesn't
  matter which of the two is shut down, as long as one is shut down
  and the other is started up; the shutdown is ordered before the
  start-up in all cases.

So, NM is waiting NM-dispatcher to start and NM-dispatcher is queued
by systemd, waiting that NM is stopped. The result is a 90 seconds
delay, after which systemd kills NM and continues.

The dependency was added so that during shutdown NM-dispatcher would
be stopped after NM. I don't think it worked as expected because
NM-dispatcher is not supposed to be active most of the times, and so
it doesn't need a dependency that delays its stop after NM.

This reverts commit acc335aa.

Move the assertion for valid LIST first. It only checks static data,
and regardless of the entry_cmd, it should be done first.

Fixes: f4d12f7b ('shared: add NM_UTILS_STRING_TABLE_LOOKUP_STRUCT_DEFINE() macro for lookup of structs')

Fixes: 190a8ed4 ('shared: add nm_ref_string_equals_str() helper')

https://mail.gnome.org/archives/networkmanager-list/2020-March/msg00000.html

Just looking at the hashtable entry of 'updelay' and 'downdelay' options
is wrong, we have to inspect their values to check if they're
actually enabled or not.

Otherwise bond connections with valid settings will fail
when created:

$ nmcli c add type bond ifname bond99 bond.options miimon=0,updelay=0,mode=0
Error: Failed to add 'bond-bond99' connection: bond.options: 'updelay' option requires 'miimon' option to be set

Also add unit tests.

https://bugzilla.redhat.com/show_bug.cgi?id=1805184

Fixes: d595f784 ('libnm: add libnm/libnm-core (part 1)')

https://mail.gnome.org/archives/networkmanager-list/2020-February/msg00040.html

Or patterns is to have the property get/set functions before
the object's create/destroy code. Move it.

The name of the property name should resemble the define for the
name.

These are special. Their setter gets called via D-Bus' SetProperty.
Mark them with a comment.

It's not necessary, nor used, nor actually implemented.

It's not necessary nor used.

It's not necessary nor used.

NetworkManager/NetworkManager!416

Don't spread the validation for the interface name between multiple
places. There should be one place only, so when you search for how
this property gets verified, you can find the single place.

That requires to move the special handling for OVS interfaces to
NMSettingConnection.
Since we already have _nm_setting_ovs_interface_verify_interface_type(),
that is easy.

Depending on the type, OVS interfaces also have a corresponding netdev
in kernel (e.g. type "internal" does, type "patch" does not).

Such a case is neither NMU_IFACE_OVS nor NMU_IFACE_KERNEL (alone). There should
be a special type to represent those cases.

Add NMU_IFACE_OVS_OR_KERNEL for that.

nm_utils_ifname_valid() is to validate "connection.interface-name"
property. But the exact validation depends on the connection type.

Add "NMU_IFACE_ANY" to validate the name to check whether it would be
valid for any connection type.

This is for completeness and for places where the caller might not know
the connection type.