From b5f64ff493728007965d1c1c8a52553b3bcbc0d2 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 13 May 2020 13:57:35 +0200 Subject: [PATCH 1/3] move tc parsing out of nm-device.c The logic to create platform qdiscs from a setting does not belong to NMDevice. Move it to NetworkManagerUtils.h. (cherry picked from commit 283f7d0b308ed62a5634ac724819876e492f4c83) --- src/NetworkManagerUtils.c | 127 ++++++++++++++++++++++++++++++++++++++ src/NetworkManagerUtils.h | 7 +++ src/devices/nm-device.c | 121 ++---------------------------------- 3 files changed, 140 insertions(+), 115 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 3f2f44d3cd..f90ea2cc54 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -9,6 +9,7 @@ #include "NetworkManagerUtils.h" #include +#include #include "nm-glib-aux/nm-c-list.h" @@ -18,6 +19,7 @@ #include "nm-setting-ip4-config.h" #include "nm-setting-ip6-config.h" #include "nm-core-internal.h" +#include "platform/nmp-object.h" #include "platform/nm-platform.h" #include "nm-auth-utils.h" @@ -1125,3 +1127,128 @@ nm_utils_file_is_in_path (const char *abs_filename, ? path : NULL; } + +/* The returned qdisc array is valid as long as s_tc is not modified */ +GPtrArray * +nm_utils_qdiscs_from_tc_setting (NMPlatform *platform, + NMSettingTCConfig *s_tc, + int ip_ifindex) +{ + GPtrArray *qdiscs; + guint nqdiscs; + guint i; + + nqdiscs = nm_setting_tc_config_get_num_qdiscs (s_tc); + qdiscs = g_ptr_array_new_full (nqdiscs, (GDestroyNotify) nmp_object_unref); + + for (i = 0; i < nqdiscs; i++) { + NMTCQdisc *s_qdisc = nm_setting_tc_config_get_qdisc (s_tc, i); + NMPObject *q = nmp_object_new (NMP_OBJECT_TYPE_QDISC, NULL); + NMPlatformQdisc *qdisc = NMP_OBJECT_CAST_QDISC (q); + + qdisc->ifindex = ip_ifindex; + qdisc->kind = nm_tc_qdisc_get_kind (s_qdisc); + + qdisc->addr_family = AF_UNSPEC; + qdisc->handle = nm_tc_qdisc_get_handle (s_qdisc); + qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc); + qdisc->info = 0; + +#define GET_ATTR(name, dst, variant_type, type, dflt) G_STMT_START { \ + GVariant *_variant = nm_tc_qdisc_get_attribute (s_qdisc, ""name""); \ + \ + if ( _variant \ + && g_variant_is_of_type (_variant, G_VARIANT_TYPE_ ## variant_type)) \ + (dst) = g_variant_get_ ## type (_variant); \ + else \ + (dst) = (dflt); \ +} G_STMT_END + + if (strcmp (qdisc->kind, "fq_codel") == 0) { + GET_ATTR ("limit", qdisc->fq_codel.limit, UINT32, uint32, 0); + GET_ATTR ("flows", qdisc->fq_codel.flows, UINT32, uint32, 0); + GET_ATTR ("target", qdisc->fq_codel.target, UINT32, uint32, 0); + GET_ATTR ("interval", qdisc->fq_codel.interval, UINT32, uint32, 0); + GET_ATTR ("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0); + GET_ATTR ("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED); + GET_ATTR ("memory_limit", qdisc->fq_codel.memory_limit, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET); + GET_ATTR ("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE); + } + +#undef GET_ADDR + + g_ptr_array_add (qdiscs, q); + } + + return qdiscs; +} + +/* The returned tfilter array is valid as long as s_tc is not modified */ +GPtrArray * +nm_utils_tfilters_from_tc_setting (NMPlatform *platform, + NMSettingTCConfig *s_tc, + int ip_ifindex) +{ + GPtrArray *tfilters; + guint ntfilters; + guint i; + + ntfilters = nm_setting_tc_config_get_num_tfilters (s_tc); + tfilters = g_ptr_array_new_full (ntfilters, (GDestroyNotify) nmp_object_unref); + + for (i = 0; i < ntfilters; i++) { + NMTCTfilter *s_tfilter = nm_setting_tc_config_get_tfilter (s_tc, i); + NMTCAction *action; + NMPObject *t = nmp_object_new (NMP_OBJECT_TYPE_TFILTER, NULL); + NMPlatformTfilter *tfilter = NMP_OBJECT_CAST_TFILTER (t); + + tfilter->ifindex = ip_ifindex; + tfilter->kind = nm_tc_tfilter_get_kind (s_tfilter); + tfilter->addr_family = AF_UNSPEC; + tfilter->handle = nm_tc_tfilter_get_handle (s_tfilter); + tfilter->parent = nm_tc_tfilter_get_parent (s_tfilter); + tfilter->info = TC_H_MAKE (0, htons (ETH_P_ALL)); + + action = nm_tc_tfilter_get_action (s_tfilter); + if (action) { + GVariant *var; + + tfilter->action.kind = nm_tc_action_get_kind (action); + + if (strcmp (tfilter->action.kind, "simple") == 0) { + var = nm_tc_action_get_attribute (action, "sdata"); + if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) { + g_strlcpy (tfilter->action.simple.sdata, + g_variant_get_bytestring (var), + sizeof (tfilter->action.simple.sdata)); + } + } else if (strcmp (tfilter->action.kind, "mirred") == 0) { + if (nm_tc_action_get_attribute (action, "egress")) + tfilter->action.mirred.egress = TRUE; + + if (nm_tc_action_get_attribute (action, "ingress")) + tfilter->action.mirred.ingress = TRUE; + + if (nm_tc_action_get_attribute (action, "mirror")) + tfilter->action.mirred.mirror = TRUE; + + if (nm_tc_action_get_attribute (action, "redirect")) + tfilter->action.mirred.redirect = TRUE; + + var = nm_tc_action_get_attribute (action, "dev"); + if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_STRING)) { + int ifindex; + + ifindex = nm_platform_link_get_ifindex (platform, + g_variant_get_string (var, NULL)); + if (ifindex > 0) + tfilter->action.mirred.ifindex = ifindex; + } + } + } + + g_ptr_array_add (tfilters, t); + } + + return tfilters; +} diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index 558a262b98..38386c3dab 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -134,4 +134,11 @@ nm_utils_file_is_in_path (const char *abs_filename, /*****************************************************************************/ +GPtrArray *nm_utils_qdiscs_from_tc_setting (NMPlatform *platform, + NMSettingTCConfig *s_tc, + int ip_ifindex); +GPtrArray *nm_utils_tfilters_from_tc_setting (NMPlatform *platform, + NMSettingTCConfig *s_tc, + int ip_ifindex); + #endif /* __NETWORKMANAGER_UTILS_H__ */ diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 63730a1087..c0aabac9ae 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -19,7 +19,6 @@ #include #include #include -#include #include "nm-std-aux/unaligned.h" #include "nm-glib-aux/nm-dedup-multi.h" @@ -6959,10 +6958,10 @@ tc_commit (NMDevice *self) gs_unref_ptrarray GPtrArray *qdiscs = NULL; gs_unref_ptrarray GPtrArray *tfilters = NULL; NMSettingTCConfig *s_tc = NULL; + NMPlatform *platform; int ip_ifindex; - guint nqdiscs, ntfilters; - guint i; + platform = nm_device_get_platform (self); connection = nm_device_get_applied_connection (self); if (connection) s_tc = nm_connection_get_setting_tc_config (connection); @@ -6972,122 +6971,14 @@ tc_commit (NMDevice *self) return s_tc == NULL; if (s_tc) { - nqdiscs = nm_setting_tc_config_get_num_qdiscs (s_tc); - qdiscs = g_ptr_array_new_full (nqdiscs, (GDestroyNotify) nmp_object_unref); - - for (i = 0; i < nqdiscs; i++) { - NMTCQdisc *s_qdisc = nm_setting_tc_config_get_qdisc (s_tc, i); - NMPObject *q = nmp_object_new (NMP_OBJECT_TYPE_QDISC, NULL); - NMPlatformQdisc *qdisc = NMP_OBJECT_CAST_QDISC (q); - - qdisc->ifindex = ip_ifindex; - - /* Note: kind string is still owned by NMTCTfilter. - * This qdisc instance must not be kept alive beyond this function. - * nm_platform_qdisc_sync() promises to do that. */ - qdisc->kind = nm_tc_qdisc_get_kind (s_qdisc); - - qdisc->addr_family = AF_UNSPEC; - qdisc->handle = nm_tc_qdisc_get_handle (s_qdisc); - qdisc->parent = nm_tc_qdisc_get_parent (s_qdisc); - qdisc->info = 0; - -#define GET_ATTR(name, dst, variant_type, type, dflt) G_STMT_START { \ - GVariant *_variant = nm_tc_qdisc_get_attribute (s_qdisc, ""name""); \ - \ - if ( _variant \ - && g_variant_is_of_type (_variant, G_VARIANT_TYPE_ ## variant_type)) \ - (dst) = g_variant_get_ ## type (_variant); \ - else \ - (dst) = (dflt); \ -} G_STMT_END - - if (strcmp (qdisc->kind, "fq_codel") == 0) { - GET_ATTR ("limit", qdisc->fq_codel.limit, UINT32, uint32, 0); - GET_ATTR ("flows", qdisc->fq_codel.flows, UINT32, uint32, 0); - GET_ATTR ("target", qdisc->fq_codel.target, UINT32, uint32, 0); - GET_ATTR ("interval", qdisc->fq_codel.interval, UINT32, uint32, 0); - GET_ATTR ("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0); - GET_ATTR ("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED); - GET_ATTR ("memory_limit", qdisc->fq_codel.memory_limit, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET); - GET_ATTR ("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE); - } - -#undef GET_ADDR - - g_ptr_array_add (qdiscs, q); - } - - ntfilters = nm_setting_tc_config_get_num_tfilters (s_tc); - tfilters = g_ptr_array_new_full (ntfilters, (GDestroyNotify) nmp_object_unref); - - for (i = 0; i < ntfilters; i++) { - NMTCTfilter *s_tfilter = nm_setting_tc_config_get_tfilter (s_tc, i); - NMTCAction *action; - NMPObject *q = nmp_object_new (NMP_OBJECT_TYPE_TFILTER, NULL); - NMPlatformTfilter *tfilter = NMP_OBJECT_CAST_TFILTER (q); - - tfilter->ifindex = ip_ifindex; - - /* Note: kind string is still owned by NMTCTfilter. - * This tfilter instance must not be kept alive beyond this function. - * nm_platform_tfilter_sync() promises to do that. */ - tfilter->kind = nm_tc_tfilter_get_kind (s_tfilter); - - tfilter->addr_family = AF_UNSPEC; - tfilter->handle = nm_tc_tfilter_get_handle (s_tfilter); - tfilter->parent = nm_tc_tfilter_get_parent (s_tfilter); - tfilter->info = TC_H_MAKE (0, htons (ETH_P_ALL)); - - action = nm_tc_tfilter_get_action (s_tfilter); - if (action) { - GVariant *var; - - /* Note: kind string is still owned by NMTCAction. - * This tfilter instance must not be kept alive beyond this function. - * nm_platform_tfilter_sync() promises to do that. */ - tfilter->action.kind = nm_tc_action_get_kind (action); - - if (strcmp (tfilter->action.kind, "simple") == 0) { - var = nm_tc_action_get_attribute (action, "sdata"); - if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_BYTESTRING)) { - g_strlcpy (tfilter->action.simple.sdata, - g_variant_get_bytestring (var), - sizeof (tfilter->action.simple.sdata)); - } - } else if (strcmp (tfilter->action.kind, "mirred") == 0) { - if (nm_tc_action_get_attribute (action, "egress")) - tfilter->action.mirred.egress = TRUE; - - if (nm_tc_action_get_attribute (action, "ingress")) - tfilter->action.mirred.ingress = TRUE; - - if (nm_tc_action_get_attribute (action, "mirror")) - tfilter->action.mirred.mirror = TRUE; - - if (nm_tc_action_get_attribute (action, "redirect")) - tfilter->action.mirred.redirect = TRUE; - - var = nm_tc_action_get_attribute (action, "dev"); - if (var && g_variant_is_of_type (var, G_VARIANT_TYPE_STRING)) { - int ifindex; - - ifindex = nm_platform_link_get_ifindex (nm_device_get_platform (self), - g_variant_get_string (var, NULL)); - if (ifindex > 0) - tfilter->action.mirred.ifindex = ifindex; - } - } - } - - g_ptr_array_add (tfilters, q); - } + qdiscs = nm_utils_qdiscs_from_tc_setting (platform, s_tc, ip_ifindex); + tfilters = nm_utils_tfilters_from_tc_setting (platform, s_tc, ip_ifindex); } - if (!nm_platform_qdisc_sync (nm_device_get_platform (self), ip_ifindex, qdiscs)) + if (!nm_platform_qdisc_sync (platform, ip_ifindex, qdiscs)) return FALSE; - if (!nm_platform_tfilter_sync (nm_device_get_platform (self), ip_ifindex, tfilters)) + if (!nm_platform_tfilter_sync (platform, ip_ifindex, tfilters)) return FALSE; return TRUE; From c3b6a44ef6df93e5625b9e07a5840bb78109b18e Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 25 May 2021 16:58:28 +0200 Subject: [PATCH 2/3] core,libnm: don't touch device TC configuration by default NetworkManager supports a very limited set of qdiscs. If users want to configure a unsupported qdisc, they need to do it outside of NetworkManager using tc. The problem is that NM also removes all qdiscs and filters during activation if the connection doesn't contain a TC setting. Therefore, setting TC configuration outside of NM is hard because users need to do it *after* the connection is up (for example through a dispatcher script). Let NM consider the presence (or absence) of a TC setting in the connection to determine whether NM should configure (or not) qdiscs and filters on the interface. We already do something similar for SR-IOV configuration. Since new connections don't have the TC setting, the new behavior (ignore existing configuration) will be the default. The impact of this change in different scenarios is: - the user previously configured TC settings via NM. This continues to work as before; - the user didn't set any qdiscs or filters in the connection, and expected NM to clear them from the interface during activation. Here there is a change in behavior, but it seems unlikely that anybody relied on the old one; - the user didn't care about qdiscs and filters; NM removed all qdiscs upon activation, and so the default qdisc from kernel was used. After this change, NM will not touch qdiscs and the default qdisc will be used, as before; - the user set a different qdisc via tc and NM cleared it during activation. Now this will work as expected. So, the new default behavior seems better than the previous one. https://bugzilla.redhat.com/show_bug.cgi?id=1928078 (cherry picked from commit a48edd0410c878d65fc5adcd5192b116ab6f8afc) (cherry picked from commit 2a8181bcd78d055b7cb9e6c0e026bc3b08231b5a) (cherry picked from commit de1449375ad0259af868a4dfa41118bef56e8493) (cherry picked from commit 97620ec18b1c325625bff06a2b8e05764c3a29ad) --- clients/common/settings-docs.h.in | 4 ++-- libnm-core/nm-setting-tc-config.c | 16 ++++++++++++++++ src/devices/nm-device.c | 26 +++++++++++++------------- 3 files changed, 31 insertions(+), 15 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index e99597aa07..26f95f6e0a 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -320,8 +320,8 @@ #define DESCRIBE_DOC_NM_SETTING_SRIOV_AUTOPROBE_DRIVERS N_("Whether to autoprobe virtual functions by a compatible driver. If set to NM_TERNARY_TRUE (1), the kernel will try to bind VFs to a compatible driver and if this succeeds a new network interface will be instantiated for each VF. If set to NM_TERNARY_FALSE (0), VFs will not be claimed and no network interfaces will be created for them. When set to NM_TERNARY_DEFAULT (-1), the global default is used; in case the global default is unspecified it is assumed to be NM_TERNARY_TRUE (1).") #define DESCRIBE_DOC_NM_SETTING_SRIOV_TOTAL_VFS N_("The total number of virtual functions to create. Note that when the sriov setting is present NetworkManager enforces the number of virtual functions on the interface (also when it is zero) during activation and resets it upon deactivation. To prevent any changes to SR-IOV parameters don't add a sriov setting to the connection.") #define DESCRIBE_DOC_NM_SETTING_SRIOV_VFS N_("Array of virtual function descriptors. Each VF descriptor is a dictionary mapping attribute names to GVariant values. The 'index' entry is mandatory for each VF. When represented as string a VF is in the form: \"INDEX [ATTR=VALUE[ ATTR=VALUE]...]\". for example: \"2 mac=00:11:22:33:44:55 spoof-check=true\". Multiple VFs can be specified using a comma as separator. Currently the following attributes are supported: mac, spoof-check, trust, min-tx-rate, max-tx-rate, vlans. The \"vlans\" attribute is represented as a semicolon-separated list of VLAN descriptors, where each descriptor has the form \"ID[.PRIORITY[.PROTO]]\". PROTO can be either 'q' for 802.1Q (the default) or 'ad' for 802.1ad.") -#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_QDISCS N_("Array of TC queueing disciplines.") -#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_TFILTERS N_("Array of TC traffic filters.") +#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_QDISCS N_("Array of TC queueing disciplines. When the \"tc\" setting is present, qdiscs from this property are applied upon activation. If the property is empty, all qdiscs are removed and the device will only have the default qdisc assigned by kernel according to the \"net.core.default_qdisc\" sysctl. If the \"tc\" setting is not present, NetworkManager doesn't touch the qdiscs present on the interface.") +#define DESCRIBE_DOC_NM_SETTING_TC_CONFIG_TFILTERS N_("Array of TC traffic filters. When the \"tc\" setting is present, filters from this property are applied upon activation. If the property is empty, NetworkManager removes all the filters. If the \"tc\" setting is not present, NetworkManager doesn't touch the filters present on the interface.") #define DESCRIBE_DOC_NM_SETTING_TEAM_CONFIG N_("The JSON configuration for the team network interface. The property should contain raw JSON configuration data suitable for teamd, because the value is passed directly to teamd. If not specified, the default configuration is used. See man teamd.conf for the format details.") #define DESCRIBE_DOC_NM_SETTING_TEAM_LINK_WATCHERS N_("Link watchers configuration for the connection: each link watcher is defined by a dictionary, whose keys depend upon the selected link watcher. Available link watchers are 'ethtool', 'nsna_ping' and 'arp_ping' and it is specified in the dictionary with the key 'name'. Available keys are: ethtool: 'delay-up', 'delay-down', 'init-wait'; nsna_ping: 'init-wait', 'interval', 'missed-max', 'target-host'; arp_ping: all the ones in nsna_ping and 'source-host', 'validate-active', 'validate-inactive', 'send-always'. See teamd.conf man for more details.") #define DESCRIBE_DOC_NM_SETTING_TEAM_MCAST_REJOIN_COUNT N_("Corresponds to the teamd mcast_rejoin.count.") diff --git a/libnm-core/nm-setting-tc-config.c b/libnm-core/nm-setting-tc-config.c index 8658231562..0b93b63cd5 100644 --- a/libnm-core/nm-setting-tc-config.c +++ b/libnm-core/nm-setting-tc-config.c @@ -1786,6 +1786,15 @@ nm_setting_tc_config_class_init (NMSettingTCConfigClass *klass) * NMSettingTCConfig:qdiscs: (type GPtrArray(NMTCQdisc)) * * Array of TC queueing disciplines. + * + * When the #NMSettingTCConfig setting is present, qdiscs from this + * property are applied upon activation. If the property is empty, + * all qdiscs are removed and the device will only + * have the default qdisc assigned by kernel according to the + * "net.core.default_qdisc" sysctl. + * + * If the #NMSettingTCConfig setting is not present, NetworkManager + * doesn't touch the qdiscs present on the interface. **/ /* ---ifcfg-rh--- * property: qdiscs @@ -1812,6 +1821,13 @@ nm_setting_tc_config_class_init (NMSettingTCConfigClass *klass) * NMSettingTCConfig:tfilters: (type GPtrArray(NMTCTfilter)) * * Array of TC traffic filters. + * + * When the #NMSettingTCConfig setting is present, filters from this + * property are applied upon activation. If the property is empty, + * NetworkManager removes all the filters. + * + * If the #NMSettingTCConfig setting is not present, NetworkManager + * doesn't touch the filters present on the interface. **/ /* ---ifcfg-rh--- * property: qdiscs diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c0aabac9ae..047999c9ce 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -6954,26 +6954,23 @@ _routing_rules_sync (NMDevice *self, static gboolean tc_commit (NMDevice *self) { - NMConnection *connection = NULL; gs_unref_ptrarray GPtrArray *qdiscs = NULL; gs_unref_ptrarray GPtrArray *tfilters = NULL; - NMSettingTCConfig *s_tc = NULL; + NMSettingTCConfig *s_tc; NMPlatform *platform; int ip_ifindex; - platform = nm_device_get_platform (self); - connection = nm_device_get_applied_connection (self); - if (connection) - s_tc = nm_connection_get_setting_tc_config (connection); + s_tc = nm_device_get_applied_setting (self, NM_TYPE_SETTING_TC_CONFIG); + if (!s_tc) + return TRUE; ip_ifindex = nm_device_get_ip_ifindex (self); if (!ip_ifindex) - return s_tc == NULL; + return FALSE; - if (s_tc) { - qdiscs = nm_utils_qdiscs_from_tc_setting (platform, s_tc, ip_ifindex); - tfilters = nm_utils_tfilters_from_tc_setting (platform, s_tc, ip_ifindex); - } + platform = nm_device_get_platform (self); + qdiscs = nm_utils_qdiscs_from_tc_setting (platform, s_tc, ip_ifindex); + tfilters = nm_utils_tfilters_from_tc_setting (platform, s_tc, ip_ifindex); if (!nm_platform_qdisc_sync (platform, ip_ifindex, qdiscs)) return FALSE; @@ -15261,8 +15258,11 @@ nm_device_cleanup (NMDevice *self, NMDeviceStateReason reason, CleanupType clean nm_platform_ip_route_flush (platform, AF_UNSPEC, ifindex); nm_platform_ip_address_flush (platform, AF_UNSPEC, ifindex); - nm_platform_tfilter_sync (platform, ifindex, NULL); - nm_platform_qdisc_sync (platform, ifindex, NULL); + + if (nm_device_get_applied_setting (self, NM_TYPE_SETTING_TC_CONFIG)) { + nm_platform_tfilter_sync (platform, ifindex, NULL); + nm_platform_qdisc_sync (platform, ifindex, NULL); + } } } From c826be4f7692e9a0efaf3626a0411fb7a94058dd Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Tue, 25 May 2021 18:00:27 +0200 Subject: [PATCH 3/3] ifcfg-rh: preserve an empty tc configuration If the TC setting contains no qdiscs and filters, it is lost after a write-read cycle. Fix this by adding a new property to indicate the presence of the (empty) setting. (cherry picked from commit 6a88d4e55cf031da2b5a8458d21487a011357da4) (cherry picked from commit acf0c4df2b0fb0dc332aa929131953390998828f) (cherry picked from commit 4efcdf234d05d386314d370822f5e783071b5765) (cherry picked from commit d3ca2ed1fcbafbf05135b7f6c14bae5e090c26e1) --- Makefile.am | 1 + libnm-core/nm-setting-tc-config.c | 14 +++- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 3 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.c | 1 + .../plugins/ifcfg-rh/nms-ifcfg-rh-utils.h | 2 +- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 38 +++++---- .../ifcfg-test-tc-write-empty.cexpected | 15 ++++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 80 +++++++++++++++++++ 8 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected diff --git a/Makefile.am b/Makefile.am index 96fe24b8f0..9ba03d87ae 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3133,6 +3133,7 @@ EXTRA_DIST += \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-static-routes-legacy.cexpected \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write.cexpected \ + src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-1 \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-2 \ src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-team-master-invalid \ diff --git a/libnm-core/nm-setting-tc-config.c b/libnm-core/nm-setting-tc-config.c index 0b93b63cd5..b2b98c67b5 100644 --- a/libnm-core/nm-setting-tc-config.c +++ b/libnm-core/nm-setting-tc-config.c @@ -1798,8 +1798,11 @@ nm_setting_tc_config_class_init (NMSettingTCConfigClass *klass) **/ /* ---ifcfg-rh--- * property: qdiscs - * variable: QDISC1(+), QDISC2(+), ... - * description: Queueing disciplines + * variable: QDISC1(+), QDISC2(+), ..., TC_COMMIT(+) + * description: Queueing disciplines to set on the interface. When no + * QDISC1, QDISC2, ..., FILTER1, FILTER2, ... keys are present, + * NetworkManager doesn't touch qdiscs and filters present on the + * interface, unless TC_COMMIT is set to 'yes'. * example: QDISC1=ingress, QDISC2="root handle 1234: fq_codel" * ---end--- */ @@ -1831,8 +1834,11 @@ nm_setting_tc_config_class_init (NMSettingTCConfigClass *klass) **/ /* ---ifcfg-rh--- * property: qdiscs - * variable: FILTER1(+), FILTER2(+), ... - * description: Traffic filters + * variable: FILTER1(+), FILTER2(+), ..., TC_COMMIT(+) + * description: Traffic filters to set on the interface. When no + * QDISC1, QDISC2, ..., FILTER1, FILTER2, ... keys are present, + * NetworkManager doesn't touch qdiscs and filters present on the + * interface, unless TC_COMMIT is set to 'yes'. * example: FILTER1="parent ffff: matchall action simple sdata Input", ... * ---end--- */ diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index b6834df0b5..b1bb3f2f18 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -2477,7 +2477,8 @@ make_tc_setting (shvarFile *ifcfg) } if ( nm_setting_tc_config_get_num_qdiscs (s_tc) > 0 - || nm_setting_tc_config_get_num_tfilters (s_tc) > 0) + || nm_setting_tc_config_get_num_tfilters (s_tc) > 0 + || svGetValueBoolean(ifcfg, "TC_COMMIT", FALSE)) return NM_SETTING (s_tc); g_object_unref (s_tc); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c index 15a772bcf7..b11a9e0846 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.c @@ -936,6 +936,7 @@ const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[] = { _KEY_TYPE ("STABLE_ID", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), _KEY_TYPE ("STP", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), _KEY_TYPE ("SUBCHANNELS", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), + _KEY_TYPE ("TC_COMMIT", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), _KEY_TYPE ("TEAM_CONFIG", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), _KEY_TYPE ("TEAM_MASTER", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), _KEY_TYPE ("TEAM_MASTER_UUID", NMS_IFCFG_KEY_TYPE_IS_PLAIN ), diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h index 6c31b26965..ea8e46abfb 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h @@ -33,7 +33,7 @@ typedef struct { NMSIfcfgKeyTypeFlags key_flags; } NMSIfcfgKeyTypeInfo; -extern const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[236]; +extern const NMSIfcfgKeyTypeInfo nms_ifcfg_well_known_keys[237]; const NMSIfcfgKeyTypeInfo *nms_ifcfg_well_known_key_find_info (const char *key, gssize *out_idx); diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index e126c77fb0..139565f2d2 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -2259,46 +2259,46 @@ write_sriov_setting (NMConnection *connection, shvarFile *ifcfg) } } -static gboolean -write_tc_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) +static void +write_tc_setting (NMConnection *connection, shvarFile *ifcfg) { NMSettingTCConfig *s_tc; - guint i, num, n; + guint num_qdiscs; + guint num_filters; + guint i; + guint n; char tag[64]; s_tc = nm_connection_get_setting_tc_config (connection); if (!s_tc) - return TRUE; + return; - num = nm_setting_tc_config_get_num_qdiscs (s_tc); - for (n = 1, i = 0; i < num; i++) { + num_qdiscs = nm_setting_tc_config_get_num_qdiscs (s_tc); + for (n = 1, i = 0; i < num_qdiscs; i++) { NMTCQdisc *qdisc; gs_free char *str = NULL; qdisc = nm_setting_tc_config_get_qdisc (s_tc, i); - str = nm_utils_tc_qdisc_to_str (qdisc, error); - if (!str) - return FALSE; - + str = nm_utils_tc_qdisc_to_str (qdisc, NULL); + nm_assert (str); svSetValueStr (ifcfg, numbered_tag (tag, "QDISC", n), str); n++; } - num = nm_setting_tc_config_get_num_tfilters (s_tc); - for (n = 1, i = 0; i < num; i++) { + num_filters = nm_setting_tc_config_get_num_tfilters (s_tc); + for (n = 1, i = 0; i < num_filters; i++) { NMTCTfilter *tfilter; gs_free char *str = NULL; tfilter = nm_setting_tc_config_get_tfilter (s_tc, i); - str = nm_utils_tc_tfilter_to_str (tfilter, error); - if (!str) - return FALSE; - + str = nm_utils_tc_tfilter_to_str (tfilter, NULL); + nm_assert (str); svSetValueStr (ifcfg, numbered_tag (tag, "FILTER", n), str); n++; } - return TRUE; + if (num_qdiscs == 0 && num_filters == 0) + svSetValueBoolean (ifcfg, "TC_COMMIT", TRUE); } static gboolean @@ -3095,9 +3095,7 @@ do_write_construct (NMConnection *connection, write_sriov_setting (connection, ifcfg); - if (!write_tc_setting (connection, ifcfg, error)) - return FALSE; - + write_tc_setting (connection, ifcfg); route_path_is_svformat = utils_has_route_file_new_syntax (route_path); diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected new file mode 100644 index 0000000000..4df768b463 --- /dev/null +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test-tc-write-empty.cexpected @@ -0,0 +1,15 @@ +TYPE=Ethernet +PROXY_METHOD=none +BROWSER_ONLY=no +TC_COMMIT=yes +BOOTPROTO=none +IPADDR=1.1.1.3 +PREFIX=24 +GATEWAY=1.1.1.1 +DEFROUTE=yes +IPV4_FAILURE_FATAL=no +IPV6INIT=no +NAME="Test Write TC config" +UUID=${UUID} +DEVICE=eth0 +ONBOOT=yes diff --git a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index ffedff0104..955f00f113 100644 --- a/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -10162,6 +10162,85 @@ test_tc_read (void) g_object_unref (connection); } +static void +test_tc_write_empty (void) +{ + nmtst_auto_unlinkfile char *testfile = NULL; + gs_unref_object NMConnection *connection = NULL; + gs_unref_object NMConnection *reread = NULL; + NMSettingConnection *s_con; + NMSettingIPConfig *s_ip4; + NMSettingIPConfig *s_ip6; + NMSettingWired *s_wired; + NMSettingTCConfig *s_tc; + NMIPAddress *addr; + GError *error = NULL; + + connection = nm_simple_connection_new (); + + /* Connection setting */ + s_con = (NMSettingConnection*) nm_setting_connection_new (); + nm_connection_add_setting (connection, NM_SETTING (s_con)); + + g_object_set (s_con, + NM_SETTING_CONNECTION_ID, + "Test Write TC config", + NM_SETTING_CONNECTION_UUID, + nm_utils_uuid_generate_a (), + NM_SETTING_CONNECTION_AUTOCONNECT, + TRUE, + NM_SETTING_CONNECTION_INTERFACE_NAME, + "eth0", + NM_SETTING_CONNECTION_TYPE, + NM_SETTING_WIRED_SETTING_NAME, + NULL); + + /* Wired setting */ + s_wired = (NMSettingWired*) nm_setting_wired_new (); + nm_connection_add_setting (connection, NM_SETTING (s_wired)); + + /* IP4 setting */ + s_ip4 = (NMSettingIPConfig*) nm_setting_ip4_config_new (); + nm_connection_add_setting (connection, NM_SETTING (s_ip4)); + + g_object_set (s_ip4, + NM_SETTING_IP_CONFIG_METHOD, + NM_SETTING_IP4_CONFIG_METHOD_MANUAL, + NM_SETTING_IP_CONFIG_GATEWAY, + "1.1.1.1", + NM_SETTING_IP_CONFIG_MAY_FAIL, + TRUE, + NULL); + + addr = nm_ip_address_new (AF_INET, "1.1.1.3", 24, &error); + g_assert_no_error (error); + nm_setting_ip_config_add_address (s_ip4, addr); + nm_ip_address_unref (addr); + + /* IP6 setting */ + s_ip6 = (NMSettingIPConfig*) nm_setting_ip6_config_new (); + nm_connection_add_setting (connection, NM_SETTING (s_ip6)); + + g_object_set (s_ip6, NM_SETTING_IP_CONFIG_METHOD, + NM_SETTING_IP6_CONFIG_METHOD_IGNORE, NULL); + + /* TC setting */ + s_tc = (NMSettingTCConfig*) nm_setting_tc_config_new (); + nm_connection_add_setting (connection, NM_SETTING (s_tc)); + + nm_connection_add_setting (connection, nm_setting_proxy_new ()); + + nmtst_assert_connection_verifies_without_normalization (connection); + + _writer_new_connec_exp (connection, + TEST_SCRATCH_DIR, + TEST_IFCFG_DIR "/ifcfg-test-tc-write-empty.cexpected", &testfile); + + reread = _connection_from_file (testfile, NULL, TYPE_BOND, NULL); + + nmtst_assert_connection_equals (connection, FALSE, reread, FALSE); +} + static void test_tc_write (void) { @@ -10699,6 +10778,7 @@ int main (int argc, char **argv) g_test_add_func (TPATH "tc/read", test_tc_read); g_test_add_func (TPATH "tc/write", test_tc_write); + g_test_add_func (TPATH "tc/write_empty", test_tc_write_empty); g_test_add_func (TPATH "utils/test_well_known_keys", test_well_known_keys); g_test_add_func (TPATH "utils/test_utils_has_route_file_new_syntax", test_utils_has_route_file_new_syntax);