From c48bfdf584e81a52e254c32d13d775e6ccfdc44c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 20 May 2020 15:20:21 +0200 Subject: [PATCH 01/21] libnm: add NMUtilsPredicateStr typedef This will be used for nm_setting_option_clear_by_name(), to filter based on a name. But it is a general purpose typedef for a predicate, not tied to NMSetting or option. --- libnm-core/nm-core-types.h | 3 +++ libnm-core/nm-utils.c | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/libnm-core/nm-core-types.h b/libnm-core/nm-core-types.h index d1ce6310cd..ce59ec036e 100644 --- a/libnm-core/nm-core-types.h +++ b/libnm-core/nm-core-types.h @@ -65,4 +65,7 @@ typedef struct _NMSettingWirelessSecurity NMSettingWirelessSecurity; typedef struct _NMSettingWpan NMSettingWpan; typedef struct _NMSimpleConnection NMSimpleConnection; +NM_AVAILABLE_IN_1_26 +typedef gboolean (*NMUtilsPredicateStr) (const char *str); + #endif /* __NM_CORE_TYPES_H__ */ diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index be76ead592..bc00516821 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -48,6 +48,20 @@ /*****************************************************************************/ +/** + * NMUtilsPredicateStr: + * @str: the name to check. + * + * This function takes a string argument and returns either %TRUE or %FALSE. + * It is a general purpose predicate, for example used by nm_setting_option_clear_by_name(). + * + * Returns: %TRUE if the predicate function matches. + * + * Since: 1.26 + */ + +/*****************************************************************************/ + struct _NMSockAddrEndpoint { const char *host; guint16 port; From adcb9350893fefe29daf3d69ed7da7873fc5bdd5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 20 May 2020 16:46:25 +0200 Subject: [PATCH 02/21] ifcfg-rh: avoid setting empty "-C/-K/-G" options for ethtool settings If no options are set, we should not generate -C/-K/-G options without parameter. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 76 ++++++++++--------- ...st_write_wired_auto_negotiate_on.cexpected | 2 +- 2 files changed, 43 insertions(+), 35 deletions(-) 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 aaa330685d..4bac775bb6 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1079,6 +1079,27 @@ write_wired_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) return TRUE; } +static void +_ethtool_gstring_prepare (GString **str, + gboolean *is_first, + char cmdline_flag, + const char *iface) +{ + if (!*is_first) { + nm_assert (*str && (*str)->len > 0); + return; + } + + if (!*str) + *str = g_string_sized_new (30); + else { + nm_assert ((*str)->len > 0); + g_string_append (*str, " ; "); + } + g_string_append_printf (*str, "-%c %s", cmdline_flag, iface); + *is_first = FALSE; +} + static gboolean write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **error) { @@ -1160,7 +1181,10 @@ write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro if (s_ethtool) { NMEthtoolID ethtool_id; NMSettingConnection *s_con; - const char *iface = NULL; + const char *iface; + gboolean is_first; + guint32 u32; + NMTernary t; s_con = nm_connection_get_setting_connection (connection); if (s_con) { @@ -1172,62 +1196,46 @@ write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro || (ch >= '0' && ch <= '9') || NM_IN_SET (ch, '_')))) iface = NULL; - } - - if (!str) - str = g_string_sized_new (30); - else - g_string_append (str, " ; "); - g_string_append (str, "-K "); - g_string_append (str, iface ?: "net0"); + } else + iface = NULL; + if (!iface) + iface = "net0"; + is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_FEATURE_FIRST; ethtool_id <= _NM_ETHTOOL_ID_FEATURE_LAST; ethtool_id++) { - const NMEthtoolData *ed = nm_ethtool_data[ethtool_id]; - NMTernary val; - nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - - val = nm_setting_ethtool_get_feature (s_ethtool, ed->optname); - if (val == NM_TERNARY_DEFAULT) + t = nm_setting_ethtool_get_feature (s_ethtool, nm_ethtool_data[ethtool_id]->optname); + if (t == NM_TERNARY_DEFAULT) continue; + _ethtool_gstring_prepare (&str, &is_first, 'K', iface); g_string_append_c (str, ' '); g_string_append (str, nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - g_string_append (str, val == NM_TERNARY_TRUE ? " on" : " off"); + g_string_append (str, t != NM_TERNARY_FALSE ? " on" : " off"); } - g_string_append (str, " ; -C "); - g_string_append (str, iface ?: "net0"); - + is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_COALESCE_FIRST; ethtool_id <= _NM_ETHTOOL_ID_COALESCE_LAST; ethtool_id++) { - const NMEthtoolData *ed = nm_ethtool_data[ethtool_id]; - guint32 val; - nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - - if (!nm_setting_ethtool_get_coalesce (s_ethtool, ed->optname, &val)) + if (!nm_setting_ethtool_get_coalesce (s_ethtool, nm_ethtool_data[ethtool_id]->optname, &u32)) continue; + _ethtool_gstring_prepare (&str, &is_first, 'C', iface); g_string_append_c (str, ' '); g_string_append (str, nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - g_string_append_printf (str, " %"G_GUINT32_FORMAT, val); + g_string_append_printf (str, " %"G_GUINT32_FORMAT, u32); } - g_string_append (str, " ; -G "); - g_string_append (str, iface ?: "net0"); - + is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_RING_FIRST; ethtool_id <= _NM_ETHTOOL_ID_RING_LAST; ethtool_id++) { - const NMEthtoolData *ed = nm_ethtool_data[ethtool_id]; - guint32 val; - nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - - if (!nm_setting_ethtool_get_ring (s_ethtool, ed->optname, &val)) + if (!nm_setting_ethtool_get_ring (s_ethtool, nm_ethtool_data[ethtool_id]->optname, &u32)) continue; + _ethtool_gstring_prepare (&str, &is_first, 'G', iface); g_string_append_c (str, ' '); g_string_append (str, nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - g_string_append_printf (str, " %"G_GUINT32_FORMAT, val); + g_string_append_printf (str, " %"G_GUINT32_FORMAT, u32); } } diff --git a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_write_wired_auto_negotiate_on.cexpected b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_write_wired_auto_negotiate_on.cexpected index 644fa06754..426085765c 100644 --- a/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_write_wired_auto_negotiate_on.cexpected +++ b/src/settings/plugins/ifcfg-rh/tests/network-scripts/ifcfg-test_write_wired_auto_negotiate_on.cexpected @@ -1,7 +1,7 @@ TYPE=Ethernet PROXY_METHOD=none BROWSER_ONLY=no -ETHTOOL_OPTS="autoneg on ; -K net0 rxvlan off tx on ; -C net0 ; -G net0" +ETHTOOL_OPTS="autoneg on ; -K net0 rxvlan off tx on" BOOTPROTO=dhcp DEFROUTE=yes IPV4_FAILURE_FATAL=no From 94a603a1bcf4ececc51f6d148473568a0c6ec1eb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:31 +0200 Subject: [PATCH 03/21] platform/trivial: rename NMEthtoolCoalesceState variables to "coalesce" All other users name a similar variable "coalesce" (or "coalesce_new", "coalesce_old"). Rename the variables, to don't use different names for similar uses. --- src/platform/nm-platform-utils.c | 94 ++++++++++++++++---------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/src/platform/nm-platform-utils.c b/src/platform/nm-platform-utils.c index 3b799e2f11..34c8238cb4 100644 --- a/src/platform/nm-platform-utils.c +++ b/src/platform/nm-platform-utils.c @@ -810,7 +810,7 @@ nmp_utils_ethtool_set_features (int ifindex, static gboolean ethtool_get_coalesce (SocketHandle *shandle, - NMEthtoolCoalesceState *out_state) + NMEthtoolCoalesceState *coalesce) { struct ethtool_coalesce eth_data; @@ -821,28 +821,28 @@ ethtool_get_coalesce (SocketHandle *shandle, sizeof (struct ethtool_coalesce)) != 0) return FALSE; - out_state->rx_coalesce_usecs = eth_data.rx_coalesce_usecs; - out_state->rx_max_coalesced_frames = eth_data.rx_max_coalesced_frames; - out_state->rx_coalesce_usecs_irq = eth_data.rx_coalesce_usecs_irq; - out_state->rx_max_coalesced_frames_irq = eth_data.rx_max_coalesced_frames_irq; - out_state->tx_coalesce_usecs = eth_data.tx_coalesce_usecs; - out_state->tx_max_coalesced_frames = eth_data.tx_max_coalesced_frames; - out_state->tx_coalesce_usecs_irq = eth_data.tx_coalesce_usecs_irq; - out_state->tx_max_coalesced_frames_irq = eth_data.tx_max_coalesced_frames_irq; - out_state->stats_block_coalesce_usecs = eth_data.stats_block_coalesce_usecs; - out_state->use_adaptive_rx_coalesce = eth_data.use_adaptive_rx_coalesce; - out_state->use_adaptive_tx_coalesce = eth_data.use_adaptive_tx_coalesce; - out_state->pkt_rate_low = eth_data.pkt_rate_low; - out_state->rx_coalesce_usecs_low = eth_data.rx_coalesce_usecs_low; - out_state->rx_max_coalesced_frames_low = eth_data.rx_max_coalesced_frames_low; - out_state->tx_coalesce_usecs_low = eth_data.tx_coalesce_usecs_low; - out_state->tx_max_coalesced_frames_low = eth_data.tx_max_coalesced_frames_low; - out_state->pkt_rate_high = eth_data.pkt_rate_high; - out_state->rx_coalesce_usecs_high = eth_data.rx_coalesce_usecs_high; - out_state->rx_max_coalesced_frames_high = eth_data.rx_max_coalesced_frames_high; - out_state->tx_coalesce_usecs_high = eth_data.tx_coalesce_usecs_high; - out_state->tx_max_coalesced_frames_high = eth_data.tx_max_coalesced_frames_high; - out_state->rate_sample_interval = eth_data.rate_sample_interval; + coalesce->rx_coalesce_usecs = eth_data.rx_coalesce_usecs; + coalesce->rx_max_coalesced_frames = eth_data.rx_max_coalesced_frames; + coalesce->rx_coalesce_usecs_irq = eth_data.rx_coalesce_usecs_irq; + coalesce->rx_max_coalesced_frames_irq = eth_data.rx_max_coalesced_frames_irq; + coalesce->tx_coalesce_usecs = eth_data.tx_coalesce_usecs; + coalesce->tx_max_coalesced_frames = eth_data.tx_max_coalesced_frames; + coalesce->tx_coalesce_usecs_irq = eth_data.tx_coalesce_usecs_irq; + coalesce->tx_max_coalesced_frames_irq = eth_data.tx_max_coalesced_frames_irq; + coalesce->stats_block_coalesce_usecs = eth_data.stats_block_coalesce_usecs; + coalesce->use_adaptive_rx_coalesce = eth_data.use_adaptive_rx_coalesce; + coalesce->use_adaptive_tx_coalesce = eth_data.use_adaptive_tx_coalesce; + coalesce->pkt_rate_low = eth_data.pkt_rate_low; + coalesce->rx_coalesce_usecs_low = eth_data.rx_coalesce_usecs_low; + coalesce->rx_max_coalesced_frames_low = eth_data.rx_max_coalesced_frames_low; + coalesce->tx_coalesce_usecs_low = eth_data.tx_coalesce_usecs_low; + coalesce->tx_max_coalesced_frames_low = eth_data.tx_max_coalesced_frames_low; + coalesce->pkt_rate_high = eth_data.pkt_rate_high; + coalesce->rx_coalesce_usecs_high = eth_data.rx_coalesce_usecs_high; + coalesce->rx_max_coalesced_frames_high = eth_data.rx_max_coalesced_frames_high; + coalesce->tx_coalesce_usecs_high = eth_data.tx_coalesce_usecs_high; + coalesce->tx_max_coalesced_frames_high = eth_data.tx_max_coalesced_frames_high; + coalesce->rate_sample_interval = eth_data.rate_sample_interval; return TRUE; } @@ -871,38 +871,38 @@ nmp_utils_ethtool_get_coalesce (int ifindex, static gboolean ethtool_set_coalesce (SocketHandle *shandle, - const NMEthtoolCoalesceState *state) + const NMEthtoolCoalesceState *coalesce) { gboolean success; struct ethtool_coalesce eth_data; g_return_val_if_fail (shandle, FALSE); - g_return_val_if_fail (state, FALSE); + g_return_val_if_fail (coalesce, FALSE); eth_data = (struct ethtool_coalesce) { .cmd = ETHTOOL_SCOALESCE, - .rx_coalesce_usecs = state->rx_coalesce_usecs, - .rx_max_coalesced_frames = state->rx_max_coalesced_frames, - .rx_coalesce_usecs_irq = state->rx_coalesce_usecs_irq, - .rx_max_coalesced_frames_irq = state->rx_max_coalesced_frames_irq, - .tx_coalesce_usecs = state->tx_coalesce_usecs, - .tx_max_coalesced_frames = state->tx_max_coalesced_frames, - .tx_coalesce_usecs_irq = state->tx_coalesce_usecs_irq, - .tx_max_coalesced_frames_irq = state->tx_max_coalesced_frames_irq, - .stats_block_coalesce_usecs = state->stats_block_coalesce_usecs, - .use_adaptive_rx_coalesce = state->use_adaptive_rx_coalesce, - .use_adaptive_tx_coalesce = state->use_adaptive_tx_coalesce, - .pkt_rate_low = state->pkt_rate_low, - .rx_coalesce_usecs_low = state->rx_coalesce_usecs_low, - .rx_max_coalesced_frames_low = state->rx_max_coalesced_frames_low, - .tx_coalesce_usecs_low = state->tx_coalesce_usecs_low, - .tx_max_coalesced_frames_low = state->tx_max_coalesced_frames_low, - .pkt_rate_high = state->pkt_rate_high, - .rx_coalesce_usecs_high = state->rx_coalesce_usecs_high, - .rx_max_coalesced_frames_high = state->rx_max_coalesced_frames_high, - .tx_coalesce_usecs_high = state->tx_coalesce_usecs_high, - .tx_max_coalesced_frames_high = state->tx_max_coalesced_frames_high, - .rate_sample_interval = state->rate_sample_interval, + .rx_coalesce_usecs = coalesce->rx_coalesce_usecs, + .rx_max_coalesced_frames = coalesce->rx_max_coalesced_frames, + .rx_coalesce_usecs_irq = coalesce->rx_coalesce_usecs_irq, + .rx_max_coalesced_frames_irq = coalesce->rx_max_coalesced_frames_irq, + .tx_coalesce_usecs = coalesce->tx_coalesce_usecs, + .tx_max_coalesced_frames = coalesce->tx_max_coalesced_frames, + .tx_coalesce_usecs_irq = coalesce->tx_coalesce_usecs_irq, + .tx_max_coalesced_frames_irq = coalesce->tx_max_coalesced_frames_irq, + .stats_block_coalesce_usecs = coalesce->stats_block_coalesce_usecs, + .use_adaptive_rx_coalesce = coalesce->use_adaptive_rx_coalesce, + .use_adaptive_tx_coalesce = coalesce->use_adaptive_tx_coalesce, + .pkt_rate_low = coalesce->pkt_rate_low, + .rx_coalesce_usecs_low = coalesce->rx_coalesce_usecs_low, + .rx_max_coalesced_frames_low = coalesce->rx_max_coalesced_frames_low, + .tx_coalesce_usecs_low = coalesce->tx_coalesce_usecs_low, + .tx_max_coalesced_frames_low = coalesce->tx_max_coalesced_frames_low, + .pkt_rate_high = coalesce->pkt_rate_high, + .rx_coalesce_usecs_high = coalesce->rx_coalesce_usecs_high, + .rx_max_coalesced_frames_high = coalesce->rx_max_coalesced_frames_high, + .tx_coalesce_usecs_high = coalesce->tx_coalesce_usecs_high, + .tx_max_coalesced_frames_high = coalesce->tx_max_coalesced_frames_high, + .rate_sample_interval = coalesce->rate_sample_interval, }; success = (_ethtool_call_handle (shandle, From 1f5f84081874d53bd12188c0843b5619478b3ea5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:31 +0200 Subject: [PATCH 04/21] device: in _ethtool_coalesce_set() only fetch current coalesce settings if needed In the common case, the user doesn't set any coalesce options. Avoid always fetching the current settings, unless they are actually needed. --- src/devices/nm-device.c | 88 +++++++++++++++----------------------- src/platform/nm-platform.c | 17 ++------ src/platform/nm-platform.h | 8 ++-- 3 files changed, 43 insertions(+), 70 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 418641358d..ab87f4646c 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -866,48 +866,6 @@ _ethtool_features_set (NMDevice *self, ethtool_state->features = g_steal_pointer (&features); } -static gboolean -_ethtool_init_coalesce (NMDevice *self, - NMPlatform *platform, - NMSettingEthtool *s_ethtool, - NMEthtoolCoalesceState *coalesce) -{ - GHashTable *hash; - GHashTableIter iter; - const char *name; - GVariant *variant; - gsize n_coalesce_set = 0; - - nm_assert (self); - nm_assert (platform); - nm_assert (coalesce); - nm_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); - - hash = _nm_setting_gendata_hash (NM_SETTING (s_ethtool), FALSE); - if (!hash) - return FALSE; - - g_hash_table_iter_init (&iter, hash); - while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &variant)) { - if (!g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)) - continue; - if (!nm_ethtool_optname_is_coalesce (name)) - continue; - - if (!nm_platform_ethtool_init_coalesce (platform, - coalesce, - name, - g_variant_get_uint32(variant))) { - _LOGW (LOGD_DEVICE, "ethtool: invalid coalesce setting %s", name); - return FALSE; - } - ++n_coalesce_set; - - } - - return !!n_coalesce_set; -} - static void _ethtool_coalesce_reset (NMDevice *self, NMPlatform *platform, @@ -939,25 +897,49 @@ _ethtool_coalesce_set (NMDevice *self, { NMEthtoolCoalesceState coalesce_old; NMEthtoolCoalesceState coalesce_new; + gboolean has_old = FALSE; + GHashTable *hash; + GHashTableIter iter; + const char *name; + GVariant *variant; - nm_assert (ethtool_state); nm_assert (NM_IS_DEVICE (self)); nm_assert (NM_IS_PLATFORM (platform)); nm_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); + nm_assert (ethtool_state); + nm_assert (!ethtool_state->coalesce); - if (!nm_platform_ethtool_get_link_coalesce (platform, - ethtool_state->ifindex, - &coalesce_old)) { - _LOGW (LOGD_DEVICE, "ethtool: failure getting coalesce settings (cannot read)"); + hash = _nm_setting_gendata_hash (NM_SETTING (s_ethtool), FALSE); + if (!hash) return; + + g_hash_table_iter_init (&iter, hash); + while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &variant)) { + NMEthtoolID ethtool_id = nm_ethtool_id_get_by_name (name); + + if (!nm_ethtool_id_is_coalesce (ethtool_id)) + continue; + + if (!has_old) { + if (!nm_platform_ethtool_get_link_coalesce (platform, + ethtool_state->ifindex, + &coalesce_old)) { + _LOGW (LOGD_DEVICE, "ethtool: failure getting coalesce settings (cannot read)"); + return; + } + has_old = TRUE; + coalesce_new = coalesce_old; + } + + nm_assert (g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)); + + nm_platform_ethtool_init_coalesce (platform, + &coalesce_new, + ethtool_id, + g_variant_get_uint32 (variant)); } - coalesce_new = coalesce_old; - - if (!_ethtool_init_coalesce (self, - platform, - s_ethtool, - &coalesce_new)) + if (!has_old) return; ethtool_state->coalesce = nm_memdup (&coalesce_old, sizeof (coalesce_old)); diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 596f08542e..7b9457f7a5 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3227,20 +3227,13 @@ nm_platform_ethtool_get_link_coalesce (NMPlatform *self, return nmp_utils_ethtool_get_coalesce (ifindex, coalesce); } -gboolean +void nm_platform_ethtool_init_coalesce (NMPlatform *self, NMEthtoolCoalesceState *coalesce, - const char *option_name, + int ethtool_id, guint32 value) { - NMEthtoolID ethtool_id; - - g_return_val_if_fail (coalesce, FALSE); - g_return_val_if_fail (option_name, FALSE); - - ethtool_id = nm_ethtool_id_get_by_name (option_name); - - g_return_val_if_fail (nm_ethtool_id_is_coalesce (ethtool_id), FALSE); + nm_assert (coalesce); switch (ethtool_id) { case NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX: @@ -3310,10 +3303,8 @@ nm_platform_ethtool_init_coalesce (NMPlatform *self, coalesce->tx_coalesce_usecs_low = value; break; default: - g_return_val_if_reached (FALSE); + g_return_if_reached (); } - - return TRUE; } gboolean diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 8957de429c..041bc87448 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -1960,10 +1960,10 @@ gboolean nm_platform_ethtool_get_link_coalesce (NMPlatform *self, int ifindex, NMEthtoolCoalesceState *coalesce); -gboolean nm_platform_ethtool_init_coalesce (NMPlatform *self, - NMEthtoolCoalesceState *coalesce, - const char *option_name, - guint32 value); +void nm_platform_ethtool_init_coalesce (NMPlatform *self, + NMEthtoolCoalesceState *coalesce, + int ethtool_id, + guint32 value); gboolean nm_platform_ethtool_set_coalesce (NMPlatform *self, int ifindex, From 1f4b190934bed6aa0b5b46d0308d260b245af9a6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:31 +0200 Subject: [PATCH 05/21] platform: make states of NMEthtoolCoalesceState indexed by ethtool_id We already have NMEthtoolID to handle coalesce options in a way that is convenient programmatically. That is, we can iterate over all valid coalesce options (it's just an integer) and use that in a more generic way. If NMEthtoolCoalesceState names all fields explicitly, we need explicit code that names each coalesce option. Especially since NMEthtoolCoalesceState is an internal and intermediate data structure, this is cumbersome and unnecessary. Thereby it also fixes the issue that nm_platform_ethtool_init_coalesce() has a NMPlatform argument without actually needing it. nm_platform_ethtool_init_coalesce() does not operate on a NMPlatform instance, and should not have the appearance of being a method of NMPlatform. --- .../nm-libnm-core-intern/nm-ethtool-utils.h | 2 + src/devices/nm-device.c | 6 +- src/platform/nm-platform-utils.c | 100 +++++++++--------- src/platform/nm-platform-utils.h | 23 +--- src/platform/nm-platform.c | 80 -------------- src/platform/nm-platform.h | 5 - 6 files changed, 56 insertions(+), 160 deletions(-) diff --git a/shared/nm-libnm-core-intern/nm-ethtool-utils.h b/shared/nm-libnm-core-intern/nm-ethtool-utils.h index 0df3d3d00c..c33c3cb14a 100644 --- a/shared/nm-libnm-core-intern/nm-ethtool-utils.h +++ b/shared/nm-libnm-core-intern/nm-ethtool-utils.h @@ -108,6 +108,8 @@ typedef enum { _NM_ETHTOOL_ID_NUM = (_NM_ETHTOOL_ID_LAST - _NM_ETHTOOL_ID_FIRST + 1), } NMEthtoolID; +#define _NM_ETHTOOL_ID_COALESCE_AS_IDX(ethtool_id) ((ethtool_id) - _NM_ETHTOOL_ID_COALESCE_FIRST) + typedef enum { NM_ETHTOOL_TYPE_UNKNOWN, NM_ETHTOOL_TYPE_COALESCE, diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ab87f4646c..a1d83c26c2 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -932,11 +932,7 @@ _ethtool_coalesce_set (NMDevice *self, } nm_assert (g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)); - - nm_platform_ethtool_init_coalesce (platform, - &coalesce_new, - ethtool_id, - g_variant_get_uint32 (variant)); + coalesce_new.s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (ethtool_id)] = g_variant_get_uint32 (variant); } if (!has_old) diff --git a/src/platform/nm-platform-utils.c b/src/platform/nm-platform-utils.c index 34c8238cb4..79529f539e 100644 --- a/src/platform/nm-platform-utils.c +++ b/src/platform/nm-platform-utils.c @@ -821,32 +821,36 @@ ethtool_get_coalesce (SocketHandle *shandle, sizeof (struct ethtool_coalesce)) != 0) return FALSE; - coalesce->rx_coalesce_usecs = eth_data.rx_coalesce_usecs; - coalesce->rx_max_coalesced_frames = eth_data.rx_max_coalesced_frames; - coalesce->rx_coalesce_usecs_irq = eth_data.rx_coalesce_usecs_irq; - coalesce->rx_max_coalesced_frames_irq = eth_data.rx_max_coalesced_frames_irq; - coalesce->tx_coalesce_usecs = eth_data.tx_coalesce_usecs; - coalesce->tx_max_coalesced_frames = eth_data.tx_max_coalesced_frames; - coalesce->tx_coalesce_usecs_irq = eth_data.tx_coalesce_usecs_irq; - coalesce->tx_max_coalesced_frames_irq = eth_data.tx_max_coalesced_frames_irq; - coalesce->stats_block_coalesce_usecs = eth_data.stats_block_coalesce_usecs; - coalesce->use_adaptive_rx_coalesce = eth_data.use_adaptive_rx_coalesce; - coalesce->use_adaptive_tx_coalesce = eth_data.use_adaptive_tx_coalesce; - coalesce->pkt_rate_low = eth_data.pkt_rate_low; - coalesce->rx_coalesce_usecs_low = eth_data.rx_coalesce_usecs_low; - coalesce->rx_max_coalesced_frames_low = eth_data.rx_max_coalesced_frames_low; - coalesce->tx_coalesce_usecs_low = eth_data.tx_coalesce_usecs_low; - coalesce->tx_max_coalesced_frames_low = eth_data.tx_max_coalesced_frames_low; - coalesce->pkt_rate_high = eth_data.pkt_rate_high; - coalesce->rx_coalesce_usecs_high = eth_data.rx_coalesce_usecs_high; - coalesce->rx_max_coalesced_frames_high = eth_data.rx_max_coalesced_frames_high; - coalesce->tx_coalesce_usecs_high = eth_data.tx_coalesce_usecs_high; - coalesce->tx_max_coalesced_frames_high = eth_data.tx_max_coalesced_frames_high; - coalesce->rate_sample_interval = eth_data.rate_sample_interval; - + *coalesce = (NMEthtoolCoalesceState) { + .s = { + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS)] = eth_data.rx_coalesce_usecs, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES)] = eth_data.rx_max_coalesced_frames, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_IRQ)] = eth_data.rx_coalesce_usecs_irq, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_IRQ)] = eth_data.rx_max_coalesced_frames_irq, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS)] = eth_data.tx_coalesce_usecs, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES)] = eth_data.tx_max_coalesced_frames, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_IRQ)] = eth_data.tx_coalesce_usecs_irq, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_IRQ)] = eth_data.tx_max_coalesced_frames_irq, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_STATS_BLOCK_USECS)] = eth_data.stats_block_coalesce_usecs, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX)] = eth_data.use_adaptive_rx_coalesce, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)] = eth_data.use_adaptive_tx_coalesce, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_PKT_RATE_LOW)] = eth_data.pkt_rate_low, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_LOW)] = eth_data.rx_coalesce_usecs_low, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_LOW)] = eth_data.rx_max_coalesced_frames_low, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_LOW)] = eth_data.tx_coalesce_usecs_low, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_LOW)] = eth_data.tx_max_coalesced_frames_low, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_PKT_RATE_HIGH)] = eth_data.pkt_rate_high, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_HIGH)] = eth_data.rx_coalesce_usecs_high, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_HIGH)] = eth_data.rx_max_coalesced_frames_high, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_HIGH)] = eth_data.tx_coalesce_usecs_high, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_HIGH)] = eth_data.tx_max_coalesced_frames_high, + [_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_SAMPLE_INTERVAL)] = eth_data.rate_sample_interval, + } + }; return TRUE; } + gboolean nmp_utils_ethtool_get_coalesce (int ifindex, NMEthtoolCoalesceState *coalesce) @@ -873,36 +877,36 @@ static gboolean ethtool_set_coalesce (SocketHandle *shandle, const NMEthtoolCoalesceState *coalesce) { - gboolean success; struct ethtool_coalesce eth_data; + gboolean success; - g_return_val_if_fail (shandle, FALSE); - g_return_val_if_fail (coalesce, FALSE); + nm_assert (shandle); + nm_assert (coalesce); eth_data = (struct ethtool_coalesce) { .cmd = ETHTOOL_SCOALESCE, - .rx_coalesce_usecs = coalesce->rx_coalesce_usecs, - .rx_max_coalesced_frames = coalesce->rx_max_coalesced_frames, - .rx_coalesce_usecs_irq = coalesce->rx_coalesce_usecs_irq, - .rx_max_coalesced_frames_irq = coalesce->rx_max_coalesced_frames_irq, - .tx_coalesce_usecs = coalesce->tx_coalesce_usecs, - .tx_max_coalesced_frames = coalesce->tx_max_coalesced_frames, - .tx_coalesce_usecs_irq = coalesce->tx_coalesce_usecs_irq, - .tx_max_coalesced_frames_irq = coalesce->tx_max_coalesced_frames_irq, - .stats_block_coalesce_usecs = coalesce->stats_block_coalesce_usecs, - .use_adaptive_rx_coalesce = coalesce->use_adaptive_rx_coalesce, - .use_adaptive_tx_coalesce = coalesce->use_adaptive_tx_coalesce, - .pkt_rate_low = coalesce->pkt_rate_low, - .rx_coalesce_usecs_low = coalesce->rx_coalesce_usecs_low, - .rx_max_coalesced_frames_low = coalesce->rx_max_coalesced_frames_low, - .tx_coalesce_usecs_low = coalesce->tx_coalesce_usecs_low, - .tx_max_coalesced_frames_low = coalesce->tx_max_coalesced_frames_low, - .pkt_rate_high = coalesce->pkt_rate_high, - .rx_coalesce_usecs_high = coalesce->rx_coalesce_usecs_high, - .rx_max_coalesced_frames_high = coalesce->rx_max_coalesced_frames_high, - .tx_coalesce_usecs_high = coalesce->tx_coalesce_usecs_high, - .tx_max_coalesced_frames_high = coalesce->tx_max_coalesced_frames_high, - .rate_sample_interval = coalesce->rate_sample_interval, + .rx_coalesce_usecs = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS)], + .rx_max_coalesced_frames = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES)], + .rx_coalesce_usecs_irq = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_IRQ)], + .rx_max_coalesced_frames_irq = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_IRQ)], + .tx_coalesce_usecs = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS)], + .tx_max_coalesced_frames = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES)], + .tx_coalesce_usecs_irq = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_IRQ)], + .tx_max_coalesced_frames_irq = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_IRQ)], + .stats_block_coalesce_usecs = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_STATS_BLOCK_USECS)], + .use_adaptive_rx_coalesce = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX)], + .use_adaptive_tx_coalesce = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)], + .pkt_rate_low = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_PKT_RATE_LOW)], + .rx_coalesce_usecs_low = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_LOW)], + .rx_max_coalesced_frames_low = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_LOW)], + .tx_coalesce_usecs_low = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_LOW)], + .tx_max_coalesced_frames_low = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_LOW)], + .pkt_rate_high = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_PKT_RATE_HIGH)], + .rx_coalesce_usecs_high = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_USECS_HIGH)], + .rx_max_coalesced_frames_high = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_RX_FRAMES_HIGH)], + .tx_coalesce_usecs_high = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_USECS_HIGH)], + .tx_max_coalesced_frames_high = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_TX_FRAMES_HIGH)], + .rate_sample_interval = coalesce->s[_NM_ETHTOOL_ID_COALESCE_AS_IDX (NM_ETHTOOL_ID_COALESCE_SAMPLE_INTERVAL)], }; success = (_ethtool_call_handle (shandle, diff --git a/src/platform/nm-platform-utils.h b/src/platform/nm-platform-utils.h index 242b97f168..062c859756 100644 --- a/src/platform/nm-platform-utils.h +++ b/src/platform/nm-platform-utils.h @@ -93,28 +93,7 @@ gboolean nmp_utils_ethtool_set_features (int ifindex, gboolean do_set /* or reset */); struct _NMEthtoolCoalesceState { - guint32 rx_coalesce_usecs; - guint32 rx_max_coalesced_frames; - guint32 rx_coalesce_usecs_irq; - guint32 rx_max_coalesced_frames_irq; - guint32 tx_coalesce_usecs; - guint32 tx_max_coalesced_frames; - guint32 tx_coalesce_usecs_irq; - guint32 tx_max_coalesced_frames_irq; - guint32 stats_block_coalesce_usecs; - guint32 use_adaptive_rx_coalesce; - guint32 use_adaptive_tx_coalesce; - guint32 pkt_rate_low; - guint32 rx_coalesce_usecs_low; - guint32 rx_max_coalesced_frames_low; - guint32 tx_coalesce_usecs_low; - guint32 tx_max_coalesced_frames_low; - guint32 pkt_rate_high; - guint32 rx_coalesce_usecs_high; - guint32 rx_max_coalesced_frames_high; - guint32 tx_coalesce_usecs_high; - guint32 tx_max_coalesced_frames_high; - guint32 rate_sample_interval; + guint32 s[_NM_ETHTOOL_ID_COALESCE_NUM /* indexed by (NMEthtoolID - _NM_ETHTOOL_ID_COALESCE_FIRST) */]; }; gboolean nmp_utils_ethtool_get_coalesce (int ifindex, diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 7b9457f7a5..8d826a109f 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3227,86 +3227,6 @@ nm_platform_ethtool_get_link_coalesce (NMPlatform *self, return nmp_utils_ethtool_get_coalesce (ifindex, coalesce); } -void -nm_platform_ethtool_init_coalesce (NMPlatform *self, - NMEthtoolCoalesceState *coalesce, - int ethtool_id, - guint32 value) -{ - nm_assert (coalesce); - - switch (ethtool_id) { - case NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX: - coalesce->use_adaptive_rx_coalesce = value; - break; - case NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX: - coalesce->use_adaptive_tx_coalesce = value; - break; - case NM_ETHTOOL_ID_COALESCE_PKT_RATE_HIGH: - coalesce->pkt_rate_high = value; - break; - case NM_ETHTOOL_ID_COALESCE_PKT_RATE_LOW: - coalesce->pkt_rate_low = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_FRAMES: - coalesce->rx_max_coalesced_frames = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_FRAMES_HIGH: - coalesce->rx_max_coalesced_frames_high = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_FRAMES_IRQ: - coalesce->rx_max_coalesced_frames_irq = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_FRAMES_LOW: - coalesce->rx_max_coalesced_frames_low = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_USECS: - coalesce->rx_coalesce_usecs = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_USECS_HIGH: - coalesce->rx_coalesce_usecs_high = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_USECS_IRQ: - coalesce->rx_coalesce_usecs_irq = value; - break; - case NM_ETHTOOL_ID_COALESCE_RX_USECS_LOW: - coalesce->rx_coalesce_usecs_low = value; - break; - case NM_ETHTOOL_ID_COALESCE_SAMPLE_INTERVAL: - coalesce->rate_sample_interval = value; - break; - case NM_ETHTOOL_ID_COALESCE_STATS_BLOCK_USECS: - coalesce->stats_block_coalesce_usecs = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_FRAMES: - coalesce->tx_max_coalesced_frames = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_FRAMES_HIGH: - coalesce->tx_max_coalesced_frames_high = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_FRAMES_IRQ: - coalesce->tx_max_coalesced_frames_irq = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_FRAMES_LOW: - coalesce->tx_max_coalesced_frames_low = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_USECS: - coalesce->tx_coalesce_usecs = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_USECS_HIGH: - coalesce->tx_coalesce_usecs_high = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_USECS_IRQ: - coalesce->tx_coalesce_usecs_irq = value; - break; - case NM_ETHTOOL_ID_COALESCE_TX_USECS_LOW: - coalesce->tx_coalesce_usecs_low = value; - break; - default: - g_return_if_reached (); - } -} - gboolean nm_platform_ethtool_set_coalesce (NMPlatform *self, int ifindex, diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 041bc87448..c21190e803 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -1960,11 +1960,6 @@ gboolean nm_platform_ethtool_get_link_coalesce (NMPlatform *self, int ifindex, NMEthtoolCoalesceState *coalesce); -void nm_platform_ethtool_init_coalesce (NMPlatform *self, - NMEthtoolCoalesceState *coalesce, - int ethtool_id, - guint32 value); - gboolean nm_platform_ethtool_set_coalesce (NMPlatform *self, int ifindex, const NMEthtoolCoalesceState *coalesce); From dcb4ed2cb18857e5df0264bad68c92bb34da772f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 21:20:05 +0200 Subject: [PATCH 06/21] ethtool: add and use _NM_ETHTOOL_ID_FEATURE_AS_IDX() macro --- libnm-core/nm-setting-ethtool.c | 6 +++--- shared/nm-libnm-core-intern/nm-ethtool-utils.h | 1 + src/platform/nm-platform-utils.c | 10 +++++----- src/platform/tests/test-link.c | 10 +++++----- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index deb1d96cca..58fa720b37 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -246,9 +246,9 @@ nm_setting_ethtool_init_features (NMSettingEthtool *setting, if (!g_variant_is_of_type (variant, G_VARIANT_TYPE_BOOLEAN)) continue; - requested[ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST] = g_variant_get_boolean (variant) - ? NM_TERNARY_TRUE - : NM_TERNARY_FALSE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (ethtool_id)] = g_variant_get_boolean (variant) + ? NM_TERNARY_TRUE + : NM_TERNARY_FALSE; n_req++; } diff --git a/shared/nm-libnm-core-intern/nm-ethtool-utils.h b/shared/nm-libnm-core-intern/nm-ethtool-utils.h index c33c3cb14a..cf971edbd2 100644 --- a/shared/nm-libnm-core-intern/nm-ethtool-utils.h +++ b/shared/nm-libnm-core-intern/nm-ethtool-utils.h @@ -108,6 +108,7 @@ typedef enum { _NM_ETHTOOL_ID_NUM = (_NM_ETHTOOL_ID_LAST - _NM_ETHTOOL_ID_FIRST + 1), } NMEthtoolID; +#define _NM_ETHTOOL_ID_FEATURE_AS_IDX(ethtool_id) ((ethtool_id) - _NM_ETHTOOL_ID_FEATURE_FIRST) #define _NM_ETHTOOL_ID_COALESCE_AS_IDX(ethtool_id) ((ethtool_id) - _NM_ETHTOOL_ID_COALESCE_FIRST) typedef enum { diff --git a/src/platform/nm-platform-utils.c b/src/platform/nm-platform-utils.c index 79529f539e..6c14bc5c16 100644 --- a/src/platform/nm-platform-utils.c +++ b/src/platform/nm-platform-utils.c @@ -520,8 +520,8 @@ _ASSERT_ethtool_feature_infos (void) for (k = 0; k < i; k++) g_assert (inf->ethtool_id != _ethtool_feature_infos[k].ethtool_id); - g_assert (!found[inf->ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST]); - found[inf->ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST] = TRUE; + g_assert (!found[_NM_ETHTOOL_ID_FEATURE_AS_IDX (inf->ethtool_id)]); + found[_NM_ETHTOOL_ID_FEATURE_AS_IDX (inf->ethtool_id)] = TRUE; kstate.idx_kernel_name = inf->n_kernel_names - 1; g_assert ((guint) kstate.idx_kernel_name == (guint) (inf->n_kernel_names - 1)); @@ -611,13 +611,13 @@ ethtool_get_features (SocketHandle *shandle) nm_assert (states_plist_n < N_ETHTOOL_KERNEL_FEATURES + G_N_ELEMENTS (_ethtool_feature_infos)); - if (!states->states_indexed[info->ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST]) - states->states_indexed[info->ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST] = &states_plist0[states_plist_n]; + if (!states->states_indexed[_NM_ETHTOOL_ID_FEATURE_AS_IDX (info->ethtool_id)]) + states->states_indexed[_NM_ETHTOOL_ID_FEATURE_AS_IDX (info->ethtool_id)] = &states_plist0[states_plist_n]; ((const NMEthtoolFeatureState **) states_plist0)[states_plist_n] = kstate; states_plist_n++; } - if (states && states->states_indexed[info->ethtool_id - _NM_ETHTOOL_ID_FEATURE_FIRST]) { + if (states && states->states_indexed[_NM_ETHTOOL_ID_FEATURE_AS_IDX (info->ethtool_id)]) { nm_assert (states_plist_n < N_ETHTOOL_KERNEL_FEATURES + G_N_ELEMENTS (_ethtool_feature_infos)); nm_assert (!states_plist0[states_plist_n]); states_plist_n++; diff --git a/src/platform/tests/test-link.c b/src/platform/tests/test-link.c index 7398d2ee27..d7285c836a 100644 --- a/src/platform/tests/test-link.c +++ b/src/platform/tests/test-link.c @@ -3290,14 +3290,14 @@ test_ethtool_features_get (void) g_ptr_array_add (gfree_keeper, requested); if (i_run == 0) { - requested[NM_ETHTOOL_ID_FEATURE_RX - _NM_ETHTOOL_ID_FEATURE_FIRST] = NM_TERNARY_FALSE; - requested[NM_ETHTOOL_ID_FEATURE_TSO - _NM_ETHTOOL_ID_FEATURE_FIRST] = NM_TERNARY_FALSE; - requested[NM_ETHTOOL_ID_FEATURE_TX_TCP6_SEGMENTATION - _NM_ETHTOOL_ID_FEATURE_FIRST] = NM_TERNARY_FALSE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (NM_ETHTOOL_ID_FEATURE_RX)] = NM_TERNARY_FALSE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (NM_ETHTOOL_ID_FEATURE_TSO)] = NM_TERNARY_FALSE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (NM_ETHTOOL_ID_FEATURE_TX_TCP6_SEGMENTATION)] = NM_TERNARY_FALSE; } else if (i_run == 1) do_set = FALSE; else if (i_run == 2) { - requested[NM_ETHTOOL_ID_FEATURE_TSO - _NM_ETHTOOL_ID_FEATURE_FIRST] = NM_TERNARY_FALSE; - requested[NM_ETHTOOL_ID_FEATURE_TX_TCP6_SEGMENTATION - _NM_ETHTOOL_ID_FEATURE_FIRST] = NM_TERNARY_TRUE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (NM_ETHTOOL_ID_FEATURE_TSO)] = NM_TERNARY_FALSE; + requested[_NM_ETHTOOL_ID_FEATURE_AS_IDX (NM_ETHTOOL_ID_FEATURE_TX_TCP6_SEGMENTATION)] = NM_TERNARY_TRUE; } else if (i_run == 3) do_set = FALSE; From 501554732c2d74bebaf8efd630ef2f00d78745a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:32 +0200 Subject: [PATCH 07/21] libnm: avoid duplicate type checks in "nm-setting-ethtool.c" Don't duplicate the code that maps the option to the variant type. Also, only resolve the name to NMEthtoolID once. Multiple calls to nm_ethtool_optname_is_*() unnecessarily need to convert the string to the ethtool id multiple times. --- libnm-core/nm-setting-ethtool.c | 78 ++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 35 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 58fa720b37..03245629db 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -22,6 +22,21 @@ /*****************************************************************************/ +static const GVariantType * +get_variant_type_from_ethtool_id (NMEthtoolID ethtool_id) +{ + if (nm_ethtool_id_is_feature (ethtool_id)) + return G_VARIANT_TYPE_BOOLEAN; + + if ( nm_ethtool_id_is_coalesce (ethtool_id) + || nm_ethtool_id_is_ring (ethtool_id)) + return G_VARIANT_TYPE_UINT32; + + return NULL; +} + +/*****************************************************************************/ + /** * nm_ethtool_optname_is_feature: * @optname: (allow-none): the option name to check @@ -497,32 +512,18 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) GVariant *variant; hash = _nm_setting_gendata_hash (setting, FALSE); - if (!hash) - goto out; + return TRUE; g_hash_table_iter_init (&iter, hash); while (g_hash_table_iter_next (&iter, (gpointer *) &optname, (gpointer *) &variant)) { - if (nm_ethtool_optname_is_feature (optname)) { - if (!g_variant_is_of_type (variant, G_VARIANT_TYPE_BOOLEAN)) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("offload feature has invalid variant type")); - g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); - return FALSE; - } - } else if ( nm_ethtool_optname_is_coalesce (optname) - || nm_ethtool_optname_is_ring (optname)) { - if (!g_variant_is_of_type (variant, G_VARIANT_TYPE_UINT32)) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("setting has invalid variant type")); - g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); - return FALSE; - } - } else { + const GVariantType *variant_type; + NMEthtoolID ethtool_id; + + ethtool_id = nm_ethtool_id_get_by_name (optname); + variant_type = get_variant_type_from_ethtool_id (ethtool_id); + + if (!variant_type) { g_set_error_literal (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -530,9 +531,17 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); return FALSE; } + + if (!g_variant_is_of_type (variant, variant_type)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("setting has invalid variant type")); + g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); + return FALSE; + } } -out: return TRUE; } @@ -543,21 +552,20 @@ get_variant_type (const NMSettInfoSetting *sett_info, const char *name, GError **error) { - NMEthtoolID ethtool_id = nm_ethtool_id_get_by_name (name); + const GVariantType *variant_type; - if (nm_ethtool_id_is_feature (ethtool_id)) - return G_VARIANT_TYPE_BOOLEAN; + variant_type = get_variant_type_from_ethtool_id (nm_ethtool_id_get_by_name (name)); - if ( nm_ethtool_id_is_coalesce (ethtool_id) - || nm_ethtool_id_is_ring (ethtool_id)) - return G_VARIANT_TYPE_UINT32; + if (!variant_type) { + g_set_error (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("unknown ethtool option '%s'"), + name); + return NULL; + } - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("unknown ethtool option '%s'"), - name); - return NULL; + return variant_type; } /*****************************************************************************/ From 20a2399aa982f2cae999829805eacbab6328bb70 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:32 +0200 Subject: [PATCH 08/21] libnm: verify that ethtool coalesce options "adaptive-[rt]x" are boolean nm_setting_ethtool_set_coalesce() coerces the values to be either 0 or 1. Verification of NMSettingEthtool should ensure the same. --- libnm-core/nm-setting-ethtool.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 03245629db..82da6f969f 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -540,6 +540,19 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); return FALSE; } + + if (NM_IN_SET (ethtool_id, + NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX, + NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)) { + if (!NM_IN_SET (g_variant_get_uint32 (variant), 0, 1)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("coalesce option must be either 0 or 1")); + g_prefix_error (error, "%s.%s: ", NM_SETTING_ETHTOOL_SETTING_NAME, optname); + return FALSE; + } + } } return TRUE; From 34fc68f20a6a341d5deb977d31a035b845bb995d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 21:28:09 +0200 Subject: [PATCH 09/21] libnm: drop unused internal function nm_setting_gendata_get_all_values() This function is not used nor does it seem useful. Either you only need the names (nm_setting_gendata_get_all_names()) or you need the names and values together (_nm_setting_gendata_get_all()). Getting the values without knowing the corresponding name makes little sense. If you need it, call _nm_setting_gendata_get_all() instead. --- libnm-core/nm-core-internal.h | 2 -- libnm-core/nm-setting.c | 27 --------------------------- 2 files changed, 29 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index cc37e68df0..9a7f74db5a 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -334,8 +334,6 @@ GVariant *nm_setting_gendata_get (NMSetting *setting, const char *const*nm_setting_gendata_get_all_names (NMSetting *setting, guint *out_len); -GVariant *const*nm_setting_gendata_get_all_values (NMSetting *setting); - gboolean nm_setting_gendata_clear (NMSetting *setting, const char *optname); diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 50a277d8b8..c215b5cb58 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2524,33 +2524,6 @@ nm_setting_gendata_get_all_names (NMSetting *setting, return names; } -/** - * nm_setting_gendata_get_all_values: - * @setting: the #NMSetting - * - * Gives the number of generic data elements and optionally returns all their - * key names and values. This API is low level access and unless you know what you - * are doing, it might not be what you want. - * - * Returns: (array zero-terminated=1) (transfer none): - * A %NULL terminated array of #GVariant. If no data is present, this returns - * %NULL. The returned array and the variants are owned by %NMSetting and might be invalidated - * soon. The sort order of nm_setting_gendata_get_all_names() and nm_setting_gendata_get_all_values() - * is consistent. That means, the nth value has the nth name returned by nm_setting_gendata_get_all_names(). - * - * Since: 1.14 - **/ -GVariant *const* -nm_setting_gendata_get_all_values (NMSetting *setting) -{ - GVariant *const*values; - - g_return_val_if_fail (NM_IS_SETTING (setting), NULL); - - _nm_setting_gendata_get_all (setting, NULL, &values); - return values; -} - void _nm_setting_gendata_to_gvalue (NMSetting *setting, GValue *value) From bfe05b48f20de0ca96495fa9e5dd892890e5ca89 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 21:30:01 +0200 Subject: [PATCH 10/21] libnm: drop unused internal API _nm_setting_gendata_reset_from_hash() and _nm_setting_gendata_to_gvalue() This was intended for when the gendata hash should be converted to/from a GValue/GHashTable. This would have been used, if we also would have added a GObject property that exposes the hash. But that was never done (at least not for NMSettingEthtool and not yet). This code is not used. If you ever need it, revert the patch or implement it anew. --- libnm-core/nm-core-internal.h | 6 ---- libnm-core/nm-setting.c | 65 ----------------------------------- 2 files changed, 71 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 9a7f74db5a..b64b4709b1 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -322,12 +322,6 @@ guint _nm_setting_gendata_get_all (NMSetting *setting, const char *const**out_names, GVariant *const**out_values); -gboolean _nm_setting_gendata_reset_from_hash (NMSetting *setting, - GHashTable *new); - -void _nm_setting_gendata_to_gvalue (NMSetting *setting, - GValue *value); - GVariant *nm_setting_gendata_get (NMSetting *setting, const char *name); diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index c215b5cb58..ad050ee47b 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2524,71 +2524,6 @@ nm_setting_gendata_get_all_names (NMSetting *setting, return names; } -void -_nm_setting_gendata_to_gvalue (NMSetting *setting, - GValue *value) -{ - GenData *gendata; - GHashTable *new; - const char *key; - GVariant *val; - GHashTableIter iter; - - nm_assert (NM_IS_SETTING (setting)); - nm_assert (value); - nm_assert (G_TYPE_CHECK_VALUE_TYPE ((value), G_TYPE_HASH_TABLE)); - - new = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, (GDestroyNotify) g_variant_unref); - - gendata = _gendata_hash (setting, FALSE); - if (gendata) { - g_hash_table_iter_init (&iter, gendata->hash); - while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &val)) - g_hash_table_insert (new, g_strdup (key), g_variant_ref (val)); - } - - g_value_take_boxed (value, new); -} - -gboolean -_nm_setting_gendata_reset_from_hash (NMSetting *setting, - GHashTable *new) -{ - GenData *gendata; - GHashTableIter iter; - const char *key; - GVariant *val; - guint num; - - nm_assert (NM_IS_SETTING (setting)); - nm_assert (new); - - num = new ? g_hash_table_size (new) : 0; - - gendata = _gendata_hash (setting, num > 0); - - if (num == 0) { - if ( !gendata - || g_hash_table_size (gendata->hash) == 0) - return FALSE; - - g_hash_table_remove_all (gendata->hash); - _nm_setting_gendata_notify (setting, TRUE); - return TRUE; - } - - /* let's not bother to find out whether the new hash has any different - * content the current gendata. Just replace it. */ - g_hash_table_remove_all (gendata->hash); - if (num > 0) { - g_hash_table_iter_init (&iter, new); - while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &val)) - g_hash_table_insert (gendata->hash, g_strdup (key), g_variant_ref (val)); - } - _nm_setting_gendata_notify (setting, TRUE); - return TRUE; -} - gboolean nm_setting_gendata_get_uint32 (NMSetting *setting, const char *optname, From 618ae93b94d23b8ce6fe5e96b23e992aa15b8945 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:32 +0200 Subject: [PATCH 11/21] libnm: rename nm_setting_gendata_*() API to nm_setting_option_*() We are going to expose some of this API in libnm. The name "gendata" (for "generic data") is not very suited. Instead, call the public API nm_setting_option_*(). This also brings no naming conflict, because currently no API exists with such naming. Rename the internal API, so that it matches the API that we are going to expose next. --- libnm-core/nm-core-internal.h | 30 +++++++++++----------- libnm-core/nm-setting-ethtool.c | 32 ++++++++++++------------ libnm-core/nm-setting.c | 44 ++++++++++++++++----------------- shared/nm-keyfile/nm-keyfile.c | 6 ++--- src/devices/nm-device.c | 4 +-- 5 files changed, 58 insertions(+), 58 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index b64b4709b1..4493a47d85 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -310,35 +310,35 @@ gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue /*****************************************************************************/ -typedef gboolean (*nm_setting_gendata_filter_fcn)(const char *name); +typedef gboolean (*NMSettingOptionFilterFcn)(const char *name); -GHashTable *_nm_setting_gendata_hash (NMSetting *setting, - gboolean create_if_necessary); +GHashTable *_nm_setting_option_hash (NMSetting *setting, + gboolean create_if_necessary); -void _nm_setting_gendata_notify (NMSetting *setting, - gboolean keys_changed); +void _nm_setting_option_notify (NMSetting *setting, + gboolean keys_changed); -guint _nm_setting_gendata_get_all (NMSetting *setting, - const char *const**out_names, - GVariant *const**out_values); +guint _nm_setting_option_get_all (NMSetting *setting, + const char *const**out_names, + GVariant *const**out_values); -GVariant *nm_setting_gendata_get (NMSetting *setting, +GVariant *_nm_setting_option_get (NMSetting *setting, const char *name); -const char *const*nm_setting_gendata_get_all_names (NMSetting *setting, +const char *const*_nm_setting_option_get_all_names (NMSetting *setting, guint *out_len); -gboolean nm_setting_gendata_clear (NMSetting *setting, +gboolean _nm_setting_option_clear (NMSetting *setting, const char *optname); -gboolean nm_setting_gendata_clear_all (NMSetting *setting, - nm_setting_gendata_filter_fcn filter); +gboolean _nm_setting_option_clear_all (NMSetting *setting, + NMSettingOptionFilterFcn filter); -gboolean nm_setting_gendata_get_uint32 (NMSetting *setting, +gboolean _nm_setting_option_get_uint32 (NMSetting *setting, const char *optname, guint32 *out_value); -void nm_setting_gendata_set_uint32 (NMSetting *setting, +void _nm_setting_option_set_uint32 (NMSetting *setting, const char *optname, guint32 value); diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 82da6f969f..664b7d4d63 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -115,7 +115,7 @@ G_DEFINE_TYPE (NMSettingEthtool, nm_setting_ethtool, NM_TYPE_SETTING) static void _notify_attributes (NMSettingEthtool *self) { - _nm_setting_gendata_notify (NM_SETTING (self), TRUE); + _nm_setting_option_notify (NM_SETTING (self), TRUE); } /*****************************************************************************/ @@ -145,7 +145,7 @@ nm_setting_ethtool_get_feature (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), NM_TERNARY_DEFAULT); g_return_val_if_fail (optname && nm_ethtool_optname_is_feature (optname), NM_TERNARY_DEFAULT); - v = nm_setting_gendata_get (NM_SETTING (setting), optname); + v = _nm_setting_option_get (NM_SETTING (setting), optname); if ( v && g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) { return g_variant_get_boolean (v) @@ -183,8 +183,8 @@ nm_setting_ethtool_set_feature (NMSettingEthtool *setting, NM_TERNARY_FALSE, NM_TERNARY_TRUE)); - hash = _nm_setting_gendata_hash (NM_SETTING (setting), - value != NM_TERNARY_DEFAULT); + hash = _nm_setting_option_hash (NM_SETTING (setting), + value != NM_TERNARY_DEFAULT); if (value == NM_TERNARY_DEFAULT) { if (hash) { @@ -226,7 +226,7 @@ nm_setting_ethtool_clear_features (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (nm_setting_gendata_clear_all (NM_SETTING (setting), + if (_nm_setting_option_clear_all (NM_SETTING (setting), &nm_ethtool_optname_is_feature)) _notify_attributes (setting); } @@ -248,7 +248,7 @@ nm_setting_ethtool_init_features (NMSettingEthtool *setting, for (i = 0; i < _NM_ETHTOOL_ID_FEATURE_NUM; i++) requested[i] = NM_TERNARY_DEFAULT; - hash = _nm_setting_gendata_hash (NM_SETTING (setting), FALSE); + hash = _nm_setting_option_hash (NM_SETTING (setting), FALSE); if (!hash) return 0; @@ -294,7 +294,7 @@ nm_setting_ethtool_get_coalesce (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); g_return_val_if_fail (nm_ethtool_optname_is_coalesce (optname), FALSE); - return nm_setting_gendata_get_uint32 (NM_SETTING (setting), + return _nm_setting_option_get_uint32 (NM_SETTING (setting), optname, out_value); } @@ -330,7 +330,7 @@ nm_setting_ethtool_set_coalesce (NMSettingEthtool *setting, NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)) value = !!value; - nm_setting_gendata_set_uint32 (NM_SETTING (setting), + _nm_setting_option_set_uint32 (NM_SETTING (setting), optname, value); _notify_attributes (setting); @@ -352,7 +352,7 @@ nm_setting_ethtool_clear_coalesce (NMSettingEthtool *setting, g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); g_return_if_fail (nm_str_not_empty (optname)); - if (nm_setting_gendata_clear (NM_SETTING (setting), optname)) + if (_nm_setting_option_clear (NM_SETTING (setting), optname)) _notify_attributes (setting); } @@ -369,7 +369,7 @@ nm_setting_ethtool_clear_coalesce_all (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (nm_setting_gendata_clear_all (NM_SETTING (setting), + if (_nm_setting_option_clear_all (NM_SETTING (setting), &nm_ethtool_optname_is_coalesce)) _notify_attributes (setting); } @@ -398,7 +398,7 @@ nm_setting_ethtool_get_ring (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); g_return_val_if_fail (nm_ethtool_optname_is_ring (optname), FALSE); - return nm_setting_gendata_get_uint32 (NM_SETTING (setting), + return _nm_setting_option_get_uint32 (NM_SETTING (setting), optname, out_value); } @@ -429,7 +429,7 @@ nm_setting_ethtool_set_ring (NMSettingEthtool *setting, g_return_if_fail (nm_ethtool_id_is_ring (ethtool_id)); - nm_setting_gendata_set_uint32 (NM_SETTING (setting), + _nm_setting_option_set_uint32 (NM_SETTING (setting), optname, value); _notify_attributes (setting); @@ -451,7 +451,7 @@ nm_setting_ethtool_clear_ring (NMSettingEthtool *setting, g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); g_return_if_fail (nm_str_not_empty (optname)); - if (nm_setting_gendata_clear (NM_SETTING (setting), optname)) + if (_nm_setting_option_clear (NM_SETTING (setting), optname)) _notify_attributes (setting); } @@ -468,7 +468,7 @@ nm_setting_ethtool_clear_ring_all (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (nm_setting_gendata_clear_all (NM_SETTING (setting), + if (_nm_setting_option_clear_all (NM_SETTING (setting), &nm_ethtool_optname_is_ring)) _notify_attributes (setting); } @@ -496,7 +496,7 @@ nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, { g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), NULL); - return nm_utils_strdict_get_keys (_nm_setting_gendata_hash (NM_SETTING (setting), FALSE), + return nm_utils_strdict_get_keys (_nm_setting_option_hash (NM_SETTING (setting), FALSE), TRUE, out_length); } @@ -511,7 +511,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) const char *optname; GVariant *variant; - hash = _nm_setting_gendata_hash (setting, FALSE); + hash = _nm_setting_option_hash (setting, FALSE); if (!hash) return TRUE; diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index ad050ee47b..c7cab45d29 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -715,7 +715,7 @@ _nm_setting_to_dbus (NMSetting *setting, g_variant_builder_init (&builder, NM_VARIANT_TYPE_SETTING); - n_properties = _nm_setting_gendata_get_all (setting, &gendata_keys, NULL); + n_properties = _nm_setting_option_get_all (setting, &gendata_keys, NULL); for (i = 0; i < n_properties; i++) { g_variant_builder_add (&builder, "{sv}", @@ -873,7 +873,7 @@ init_from_dbus (NMSetting *setting, g_hash_table_remove (keys, key); } - _nm_setting_gendata_notify (setting, TRUE); + _nm_setting_option_notify (setting, TRUE); /* Currently, only NMSettingEthtool supports gendata based options, and * that one has no other properties (except "name"). That means, we @@ -1792,7 +1792,7 @@ nm_setting_enumerate_values (NMSetting *setting, /* the properties of this setting are not real GObject properties. * Hence, this API makes little sense (or does it?). Still, call * @func with each value. */ - n_properties = _nm_setting_gendata_get_all (setting, &names, NULL); + n_properties = _nm_setting_option_get_all (setting, &names, NULL); if (n_properties > 0) { gs_strfreev char **keys = g_strdupv ((char **) names); GHashTable *h = _gendata_hash (setting, FALSE)->hash; @@ -2389,7 +2389,7 @@ _gendata_hash (NMSetting *setting, gboolean create_if_necessary) } GHashTable * -_nm_setting_gendata_hash (NMSetting *setting, gboolean create_if_necessary) +_nm_setting_option_hash (NMSetting *setting, gboolean create_if_necessary) { GenData *gendata; @@ -2398,8 +2398,8 @@ _nm_setting_gendata_hash (NMSetting *setting, gboolean create_if_necessary) } void -_nm_setting_gendata_notify (NMSetting *setting, - gboolean names_changed) +_nm_setting_option_notify (NMSetting *setting, + gboolean names_changed) { GenData *gendata; @@ -2434,7 +2434,7 @@ out: } GVariant * -nm_setting_gendata_get (NMSetting *setting, +_nm_setting_option_get (NMSetting *setting, const char *name) { GenData *gendata; @@ -2447,9 +2447,9 @@ nm_setting_gendata_get (NMSetting *setting, } guint -_nm_setting_gendata_get_all (NMSetting *setting, - const char *const**out_names, - GVariant *const**out_values) +_nm_setting_option_get_all (NMSetting *setting, + const char *const**out_names, + GVariant *const**out_values) { GenData *gendata; GHashTable *hash; @@ -2495,7 +2495,7 @@ out_zero: } /** - * nm_setting_gendata_get_all_names: + * _nm_setting_option_get_all_names: * @setting: the #NMSetting * @out_len: (allow-none) (out): * @@ -2511,7 +2511,7 @@ out_zero: * Since: 1.14 **/ const char *const* -nm_setting_gendata_get_all_names (NMSetting *setting, +_nm_setting_option_get_all_names (NMSetting *setting, guint *out_len) { const char *const*names; @@ -2519,13 +2519,13 @@ nm_setting_gendata_get_all_names (NMSetting *setting, g_return_val_if_fail (NM_IS_SETTING (setting), NULL); - len = _nm_setting_gendata_get_all (setting, &names, NULL); + len = _nm_setting_option_get_all (setting, &names, NULL); NM_SET_OUT (out_len, len); return names; } gboolean -nm_setting_gendata_get_uint32 (NMSetting *setting, +_nm_setting_option_get_uint32 (NMSetting *setting, const char *optname, guint32 *out_value) { @@ -2534,7 +2534,7 @@ nm_setting_gendata_get_uint32 (NMSetting *setting, nm_assert (NM_IS_SETTING (setting)); nm_assert (nm_str_not_empty (optname)); - v = nm_setting_gendata_get (setting, optname); + v = _nm_setting_option_get (setting, optname); if ( v && g_variant_is_of_type (v, G_VARIANT_TYPE_UINT32)) { NM_SET_OUT (out_value, g_variant_get_uint32 (v)); @@ -2545,20 +2545,20 @@ nm_setting_gendata_get_uint32 (NMSetting *setting, } void -nm_setting_gendata_set_uint32 (NMSetting *setting, +_nm_setting_option_set_uint32 (NMSetting *setting, const char *optname, guint32 value) { nm_assert (NM_IS_SETTING (setting)); nm_assert (nm_str_not_empty (optname)); - g_hash_table_insert (_nm_setting_gendata_hash (setting, TRUE), + g_hash_table_insert (_nm_setting_option_hash (setting, TRUE), g_strdup (optname), g_variant_ref_sink (g_variant_new_uint32 (value))); } gboolean -nm_setting_gendata_clear (NMSetting *setting, +_nm_setting_option_clear (NMSetting *setting, const char *optname) { GHashTable *ht; @@ -2566,7 +2566,7 @@ nm_setting_gendata_clear (NMSetting *setting, nm_assert (NM_IS_SETTING (setting)); nm_assert (nm_str_not_empty (optname)); - ht = _nm_setting_gendata_hash (setting, FALSE); + ht = _nm_setting_option_hash (setting, FALSE); if (!ht) return FALSE; @@ -2574,8 +2574,8 @@ nm_setting_gendata_clear (NMSetting *setting, } gboolean -nm_setting_gendata_clear_all (NMSetting *setting, - nm_setting_gendata_filter_fcn filter) +_nm_setting_option_clear_all (NMSetting *setting, + NMSettingOptionFilterFcn filter) { GHashTable *ht; const char *name; @@ -2584,7 +2584,7 @@ nm_setting_gendata_clear_all (NMSetting *setting, nm_assert (NM_IS_SETTING (setting)); - ht = _nm_setting_gendata_hash (setting, FALSE); + ht = _nm_setting_option_hash (setting, FALSE); if (!ht) return FALSE; diff --git a/shared/nm-keyfile/nm-keyfile.c b/shared/nm-keyfile/nm-keyfile.c index 372ae202e3..35de1a727f 100644 --- a/shared/nm-keyfile/nm-keyfile.c +++ b/shared/nm-keyfile/nm-keyfile.c @@ -3195,7 +3195,7 @@ _read_setting (KeyfileReaderInfo *info) if (!keys) n_keys = 0; if (n_keys > 0) { - GHashTable *h = _nm_setting_gendata_hash (setting, TRUE); + GHashTable *h = _nm_setting_option_hash (setting, TRUE); nm_utils_strv_sort (keys, n_keys); for (k = 0; k < n_keys; k++) { @@ -3851,10 +3851,10 @@ nm_keyfile_write (NMConnection *connection, nm_assert (!nm_keyfile_plugin_get_alias_for_setting_name (sett_info->setting_class->setting_info->setting_name)); - n_keys = _nm_setting_gendata_get_all (setting, &keys, NULL); + n_keys = _nm_setting_option_get_all (setting, &keys, NULL); if (n_keys > 0) { - GHashTable *h = _nm_setting_gendata_hash (setting, FALSE); + GHashTable *h = _nm_setting_option_hash (setting, FALSE); for (k = 0; k < n_keys; k++) { const char *key = keys[k]; diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a1d83c26c2..e6fe97493a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -909,7 +909,7 @@ _ethtool_coalesce_set (NMDevice *self, nm_assert (ethtool_state); nm_assert (!ethtool_state->coalesce); - hash = _nm_setting_gendata_hash (NM_SETTING (s_ethtool), FALSE); + hash = _nm_setting_option_hash (NM_SETTING (s_ethtool), FALSE); if (!hash) return; @@ -967,7 +967,7 @@ _ethtool_init_ring (NMDevice *self, nm_assert (ring); nm_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); - hash = _nm_setting_gendata_hash (NM_SETTING (s_ethtool), FALSE); + hash = _nm_setting_option_hash (NM_SETTING (s_ethtool), FALSE); if (!hash) return FALSE; From 9655dff5cb60a0b920a3e9faed64913a10f5be65 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:32 +0200 Subject: [PATCH 12/21] libnm: add API for setting gendata options to NMSetting (nm_setting_option_get()) NMSettingEthtool is implemented using "gendata", meaning a hash of GVariant. This is different from most other settings that have properties implemented as GObject properties. There are two reasons for this approach: - The setting is transferred via D-Bus as "a{sv}" dictionary. By unpacking the dictionary into GObject properties, the setting cannot handle unknown properties. To be forward compatible (and due to sloppy programming), unknown dictionary keys are silently ignored when parsing a NMSetting. That is error prone and also prevents settings to be treated loss-less. Instead, we should at first accept all values from the dictionary. Only in a second step, nm_connection_verify() rejects invalid settings with an error reason. This way, the user can create a NMSetting, but in a separate step handle if the setting doesn't verify. "gendata" solves this by tracking the full GVariant dictionary. This is still not entirely lossless, because multiple keys are combined. This is for example interesting if an libnm client fetches a connection from a newer NetworkManager version. Now the user can modify the properties that she knows about, while leaving all unknown properties (from newer versions) in place. - the approach aims to reduce the necessary boiler plate to create GObject properties. Adding a new property should require less new code. This approach was always be intended to be suitable for all settings, not only NMSettingEthtool. We should not once again try to add API like nm_setting_ethtool_set_feature(), nm_setting_ethtool_set_coalesce(), etc. Note that the option name already fully encodes whether it is a feature, a coalesce option, or whatever. We should not have "nm_setting_set_$SUB_GROUP (setting, $ONE_NAME_FROM_GROUP)" API, but simply "nm_setting_option_set (setting, $OPTION)" accessors. Also, when parsing a NMSettingEthtool from a GVariant, then a feature option can be any kind of variant. Only nm_setting_verify() rejects variants of the wrong type. As such, nm_setting_option_set*() also doesn't validate whether the variant type matches the option. Of course, if you set a value of wrong type, verify() will reject the setting. Add new general purpose API for this and expose it for NMSetting. --- libnm-core/nm-core-internal.h | 3 --- libnm-core/nm-setting-ethtool.c | 2 +- libnm-core/nm-setting.c | 40 +++++++++++++++++++++------------ libnm-core/nm-setting.h | 7 ++++++ libnm/libnm.ver | 1 + 5 files changed, 35 insertions(+), 18 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 4493a47d85..252c61d64e 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -322,9 +322,6 @@ guint _nm_setting_option_get_all (NMSetting *setting, const char *const**out_names, GVariant *const**out_values); -GVariant *_nm_setting_option_get (NMSetting *setting, - const char *name); - const char *const*_nm_setting_option_get_all_names (NMSetting *setting, guint *out_len); diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 664b7d4d63..de7bded5f2 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -145,7 +145,7 @@ nm_setting_ethtool_get_feature (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), NM_TERNARY_DEFAULT); g_return_val_if_fail (optname && nm_ethtool_optname_is_feature (optname), NM_TERNARY_DEFAULT); - v = _nm_setting_option_get (NM_SETTING (setting), optname); + v = nm_setting_option_get (NM_SETTING (setting), optname); if ( v && g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) { return g_variant_get_boolean (v) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index c7cab45d29..efd81c7b8a 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2433,19 +2433,6 @@ out: _nm_setting_emit_property_changed (setting); } -GVariant * -_nm_setting_option_get (NMSetting *setting, - const char *name) -{ - GenData *gendata; - - g_return_val_if_fail (NM_IS_SETTING (setting), NULL); - g_return_val_if_fail (name, NULL); - - gendata = _gendata_hash (setting, FALSE); - return gendata ? g_hash_table_lookup (gendata->hash, name) : NULL; -} - guint _nm_setting_option_get_all (NMSetting *setting, const char *const**out_names, @@ -2534,7 +2521,7 @@ _nm_setting_option_get_uint32 (NMSetting *setting, nm_assert (NM_IS_SETTING (setting)); nm_assert (nm_str_not_empty (optname)); - v = _nm_setting_option_get (setting, optname); + v = nm_setting_option_get (setting, optname); if ( v && g_variant_is_of_type (v, G_VARIANT_TYPE_UINT32)) { NM_SET_OUT (out_value, g_variant_get_uint32 (v)); @@ -2601,6 +2588,31 @@ _nm_setting_option_clear_all (NMSetting *setting, /*****************************************************************************/ +/** + * nm_setting_option_get: + * @setting: the #NMSetting + * @opt_name: the option name to request. + * + * Returns: (transfer none): the #GVariant or %NULL if the option + * is not set. + * + * Since: 1.26. + */ +GVariant * +nm_setting_option_get (NMSetting *setting, + const char *opt_name) +{ + GenData *gendata; + + g_return_val_if_fail (NM_IS_SETTING (setting), FALSE); + g_return_val_if_fail (opt_name, FALSE); + + gendata = _gendata_hash (setting, FALSE); + return gendata ? g_hash_table_lookup (gendata->hash, opt_name) : NULL; +} + +/*****************************************************************************/ + static void get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index 26c1b1ee8d..a0bb51bbf0 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -333,6 +333,13 @@ gboolean nm_setting_set_secret_flags (NMSetting *setting, /*****************************************************************************/ +NM_AVAILABLE_IN_1_26 +GVariant *nm_setting_option_get (NMSetting *setting, + const char *opt_name); + + +/*****************************************************************************/ + const GVariantType *nm_setting_get_dbus_property_type (NMSetting *setting, const char *property_name); diff --git a/libnm/libnm.ver b/libnm/libnm.ver index effa41d162..97cdc06a83 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1732,4 +1732,5 @@ global: nm_setting_match_remove_driver_by_value; nm_setting_match_remove_kernel_command_line; nm_setting_match_remove_kernel_command_line_by_value; + nm_setting_option_get; } libnm_1_24_0; From d0192b698e683b40d7e8ea64dbfe5ec29fc71dad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:32 +0200 Subject: [PATCH 13/21] libnm: add nm_setting_option_set(), nm_setting_option_get_boolean(), nm_setting_option_set_boolean() More general purpose API for generic options of settings. --- libnm-core/nm-setting.c | 140 ++++++++++++++++++++++++++++++++++++++++ libnm-core/nm-setting.h | 14 ++++ libnm/libnm.ver | 3 + 3 files changed, 157 insertions(+) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index efd81c7b8a..b8406728b6 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2611,6 +2611,146 @@ nm_setting_option_get (NMSetting *setting, return gendata ? g_hash_table_lookup (gendata->hash, opt_name) : NULL; } +/** + * nm_setting_option_get_boolean: + * @setting: the #NMSetting + * @opt_name: the option to get + * @out_value: (allow-none) (out): the optional output value. + * If the option is unset, %FALSE will be returned. + * + * Returns: %TRUE if @opt_name is set to a boolean variant. + * + * Since: 1.26 + */ +gboolean +nm_setting_option_get_boolean (NMSetting *setting, + const char *opt_name, + gboolean *out_value) +{ + GVariant *v; + + v = nm_setting_option_get (NM_SETTING (setting), opt_name); + if ( v + && g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) { + NM_SET_OUT (out_value, g_variant_get_boolean (v)); + return TRUE; + } + NM_SET_OUT (out_value, FALSE); + return FALSE; +} + +/** + * nm_setting_option_set: + * @setting: the #NMSetting + * @opt_name: the option name to set + * @variant: (allow-none): the variant to set. + * + * If @variant is %NULL, this clears the option if it is set. + * Otherwise, @variant is set as the option. If @variant is + * a floating reference, it will be consumed. + * + * Note that not all setting types support options. It is a bug + * setting a variant to a setting that doesn't support it. + * Currently only #NMSettingEthtool supports it. + * + * Since: 1.26 + */ +void +nm_setting_option_set (NMSetting *setting, + const char *opt_name, + GVariant *variant) +{ + GVariant *old_variant; + gboolean changed_name; + gboolean changed_value; + GHashTable *hash; + + g_return_if_fail (NM_IS_SETTING (setting)); + g_return_if_fail (opt_name); + + hash = _nm_setting_option_hash (setting, variant != NULL); + + if (!variant) { + if (hash) { + if (g_hash_table_remove (hash, opt_name)) + _nm_setting_option_notify (setting, TRUE); + } + return; + } + + /* Currently, it is a bug setting any option, unless the setting type supports it. + * And currently, only NMSettingEthtool supports it. + * + * In the future, more setting types may support it. Or we may relax this so + * that options can be attached to all setting types (to indicate "unsupported" + * settings for forward compatibility). + * + * As it is today, internal code will only add gendata options to NMSettingEthtool, + * and there exists not public API to add such options. Still, it is permissible + * to call get(), clear() and set(variant=NULL) also on settings that don't support + * it, as these operations don't add options. + */ + g_return_if_fail (_nm_setting_class_get_sett_info (NM_SETTING_GET_CLASS (setting))->detail.gendata_info); + + old_variant = g_hash_table_lookup (hash, opt_name); + + changed_name = (old_variant == NULL); + changed_value = changed_name + || !g_variant_equal (old_variant, variant); + + /* We always want to replace the variant, even if it has + * the same value according to g_variant_equal(). The reason + * is that we want to take a reference on @variant, because + * that is what the user might expect. */ + g_hash_table_insert (hash, + g_strdup (opt_name), + g_variant_ref_sink (variant)); + + if (changed_value) + _nm_setting_option_notify (setting, !changed_name); +} + +/** + * nm_setting_option_set_boolean: + * @setting: the #NMSetting + * @value: the value to set. + * + * Like nm_setting_option_set() to set a boolean GVariant. + * + * Since: 1.26 + */ +void +nm_setting_option_set_boolean (NMSetting *setting, + const char *opt_name, + gboolean value) +{ + GVariant *old_variant; + gboolean changed_name; + gboolean changed_value; + GHashTable *hash; + + g_return_if_fail (NM_IS_SETTING (setting)); + g_return_if_fail (opt_name); + + value = (!!value); + + hash = _nm_setting_option_hash (setting, TRUE); + + old_variant = g_hash_table_lookup (hash, opt_name); + + changed_name = (old_variant == NULL); + changed_value = changed_name + || ( !g_variant_is_of_type (old_variant, G_VARIANT_TYPE_BOOLEAN) + || g_variant_get_boolean (old_variant) != value); + + g_hash_table_insert (hash, + g_strdup (opt_name), + g_variant_ref_sink (g_variant_new_boolean (value))); + + if (changed_value) + _nm_setting_option_notify (setting, !changed_name); +} + /*****************************************************************************/ static void diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index a0bb51bbf0..ed909b7b25 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -337,6 +337,20 @@ NM_AVAILABLE_IN_1_26 GVariant *nm_setting_option_get (NMSetting *setting, const char *opt_name); +NM_AVAILABLE_IN_1_26 +gboolean nm_setting_option_get_boolean (NMSetting *setting, + const char *opt_name, + gboolean *out_value); + +NM_AVAILABLE_IN_1_26 +void nm_setting_option_set (NMSetting *setting, + const char *opt_name, + GVariant *variant); + +NM_AVAILABLE_IN_1_26 +void nm_setting_option_set_boolean (NMSetting *setting, + const char *opt_name, + gboolean value); /*****************************************************************************/ diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 97cdc06a83..4f39d3917c 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1733,4 +1733,7 @@ global: nm_setting_match_remove_kernel_command_line; nm_setting_match_remove_kernel_command_line_by_value; nm_setting_option_get; + nm_setting_option_get_boolean; + nm_setting_option_set; + nm_setting_option_set_boolean; } libnm_1_24_0; From 150af44e104243ca8a629c291bb0e63373c79920 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 20 May 2020 16:31:30 +0200 Subject: [PATCH 14/21] libnm: add nm_setting_option_get_uint32(), nm_setting_option_set_uint32() More general purpose API for generic options of settings. --- libnm-core/nm-core-internal.h | 8 --- libnm-core/nm-setting-ethtool.c | 24 ++++---- libnm-core/nm-setting.c | 100 +++++++++++++++++++++----------- libnm-core/nm-setting.h | 10 ++++ libnm/libnm.ver | 2 + 5 files changed, 91 insertions(+), 53 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 252c61d64e..b1689da62e 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -331,14 +331,6 @@ gboolean _nm_setting_option_clear (NMSetting *setting, gboolean _nm_setting_option_clear_all (NMSetting *setting, NMSettingOptionFilterFcn filter); -gboolean _nm_setting_option_get_uint32 (NMSetting *setting, - const char *optname, - guint32 *out_value); - -void _nm_setting_option_set_uint32 (NMSetting *setting, - const char *optname, - guint32 value); - /*****************************************************************************/ guint nm_setting_ethtool_init_features (NMSettingEthtool *setting, diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index de7bded5f2..1be74cc9cb 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -294,9 +294,9 @@ nm_setting_ethtool_get_coalesce (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); g_return_val_if_fail (nm_ethtool_optname_is_coalesce (optname), FALSE); - return _nm_setting_option_get_uint32 (NM_SETTING (setting), - optname, - out_value); + return nm_setting_option_get_uint32 (NM_SETTING (setting), + optname, + out_value); } /** @@ -330,9 +330,9 @@ nm_setting_ethtool_set_coalesce (NMSettingEthtool *setting, NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)) value = !!value; - _nm_setting_option_set_uint32 (NM_SETTING (setting), - optname, - value); + nm_setting_option_set_uint32 (NM_SETTING (setting), + optname, + value); _notify_attributes (setting); } @@ -398,9 +398,9 @@ nm_setting_ethtool_get_ring (NMSettingEthtool *setting, g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); g_return_val_if_fail (nm_ethtool_optname_is_ring (optname), FALSE); - return _nm_setting_option_get_uint32 (NM_SETTING (setting), - optname, - out_value); + return nm_setting_option_get_uint32 (NM_SETTING (setting), + optname, + out_value); } /** @@ -429,9 +429,9 @@ nm_setting_ethtool_set_ring (NMSettingEthtool *setting, g_return_if_fail (nm_ethtool_id_is_ring (ethtool_id)); - _nm_setting_option_set_uint32 (NM_SETTING (setting), - optname, - value); + nm_setting_option_set_uint32 (NM_SETTING (setting), + optname, + value); _notify_attributes (setting); } diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index b8406728b6..98d9d9498f 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2511,39 +2511,6 @@ _nm_setting_option_get_all_names (NMSetting *setting, return names; } -gboolean -_nm_setting_option_get_uint32 (NMSetting *setting, - const char *optname, - guint32 *out_value) -{ - GVariant *v; - - nm_assert (NM_IS_SETTING (setting)); - nm_assert (nm_str_not_empty (optname)); - - v = nm_setting_option_get (setting, optname); - if ( v - && g_variant_is_of_type (v, G_VARIANT_TYPE_UINT32)) { - NM_SET_OUT (out_value, g_variant_get_uint32 (v)); - return TRUE; - } - NM_SET_OUT (out_value, 0); - return FALSE; -} - -void -_nm_setting_option_set_uint32 (NMSetting *setting, - const char *optname, - guint32 value) -{ - nm_assert (NM_IS_SETTING (setting)); - nm_assert (nm_str_not_empty (optname)); - - g_hash_table_insert (_nm_setting_option_hash (setting, TRUE), - g_strdup (optname), - g_variant_ref_sink (g_variant_new_uint32 (value))); -} - gboolean _nm_setting_option_clear (NMSetting *setting, const char *optname) @@ -2639,6 +2606,34 @@ nm_setting_option_get_boolean (NMSetting *setting, return FALSE; } +/** + * nm_setting_option_get_uint32: + * @setting: the #NMSetting + * @opt_name: the option to get + * @out_value: (allow-none) (out): the optional output value. + * If the option is unset, 0 will be returned. + * + * Returns: %TRUE if @opt_name is set to a uint32 variant. + * + * Since: 1.26 + */ +gboolean +nm_setting_option_get_uint32 (NMSetting *setting, + const char *opt_name, + guint32 *out_value) +{ + GVariant *v; + + v = nm_setting_option_get (NM_SETTING (setting), opt_name); + if ( v + && g_variant_is_of_type (v, G_VARIANT_TYPE_UINT32)) { + NM_SET_OUT (out_value, g_variant_get_uint32 (v)); + return TRUE; + } + NM_SET_OUT (out_value, 0); + return FALSE; +} + /** * nm_setting_option_set: * @setting: the #NMSetting @@ -2751,6 +2746,45 @@ nm_setting_option_set_boolean (NMSetting *setting, _nm_setting_option_notify (setting, !changed_name); } +/** + * nm_setting_option_set_uint32: + * @setting: the #NMSetting + * @value: the value to set. + * + * Like nm_setting_option_set() to set a uint32 GVariant. + * + * Since: 1.26 + */ +void +nm_setting_option_set_uint32 (NMSetting *setting, + const char *opt_name, + guint32 value) +{ + GVariant *old_variant; + gboolean changed_name; + gboolean changed_value; + GHashTable *hash; + + g_return_if_fail (NM_IS_SETTING (setting)); + g_return_if_fail (opt_name); + + hash = _nm_setting_option_hash (setting, TRUE); + + old_variant = g_hash_table_lookup (hash, opt_name); + + changed_name = (old_variant == NULL); + changed_value = changed_name + || ( !g_variant_is_of_type (old_variant, G_VARIANT_TYPE_UINT32) + || g_variant_get_uint32 (old_variant) != value); + + g_hash_table_insert (hash, + g_strdup (opt_name), + g_variant_ref_sink (g_variant_new_uint32 (value))); + + if (changed_value) + _nm_setting_option_notify (setting, !changed_name); +} + /*****************************************************************************/ static void diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index ed909b7b25..27e9208e6f 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -342,11 +342,21 @@ gboolean nm_setting_option_get_boolean (NMSetting *setting, const char *opt_name, gboolean *out_value); +NM_AVAILABLE_IN_1_26 +gboolean nm_setting_option_get_uint32 (NMSetting *setting, + const char *opt_name, + guint32 *out_value); + NM_AVAILABLE_IN_1_26 void nm_setting_option_set (NMSetting *setting, const char *opt_name, GVariant *variant); +NM_AVAILABLE_IN_1_26 +void nm_setting_option_set_uint32 (NMSetting *setting, + const char *opt_name, + guint32 value); + NM_AVAILABLE_IN_1_26 void nm_setting_option_set_boolean (NMSetting *setting, const char *opt_name, diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 4f39d3917c..b1abdc257d 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1734,6 +1734,8 @@ global: nm_setting_match_remove_kernel_command_line_by_value; nm_setting_option_get; nm_setting_option_get_boolean; + nm_setting_option_get_uint32; nm_setting_option_set; nm_setting_option_set_boolean; + nm_setting_option_set_uint32; } libnm_1_24_0; From 1a56a2105cf1b3ca18a2e889bb1edcf4539bacf7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:33 +0200 Subject: [PATCH 15/21] libnm: add nm_setting_option_get_names() More general purpose API for generic options of settings. --- libnm-core/nm-core-internal.h | 3 --- libnm-core/nm-setting.c | 14 ++++++-------- libnm-core/nm-setting.h | 4 ++++ libnm/libnm.ver | 1 + 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index b1689da62e..9fe658c1c2 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -322,9 +322,6 @@ guint _nm_setting_option_get_all (NMSetting *setting, const char *const**out_names, GVariant *const**out_values); -const char *const*_nm_setting_option_get_all_names (NMSetting *setting, - guint *out_len); - gboolean _nm_setting_option_clear (NMSetting *setting, const char *optname); diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 98d9d9498f..a99a46b0ea 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2482,24 +2482,22 @@ out_zero: } /** - * _nm_setting_option_get_all_names: + * nm_setting_option_get_all_names: * @setting: the #NMSetting * @out_len: (allow-none) (out): * - * Gives the number of generic data elements and optionally returns all their - * key names and values. This API is low level access and unless you know what you - * are doing, it might not be what you want. + * Gives the name of all set options. * * Returns: (array length=out_len zero-terminated=1) (transfer none): * A %NULL terminated array of key names. If no names are present, this returns * %NULL. The returned array and the names are owned by %NMSetting and might be invalidated - * soon. + * by the next operation. * - * Since: 1.14 + * Since: 1.26 **/ const char *const* -_nm_setting_option_get_all_names (NMSetting *setting, - guint *out_len) +nm_setting_option_get_all_names (NMSetting *setting, + guint *out_len) { const char *const*names; guint len; diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index 27e9208e6f..a3a732218e 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -362,6 +362,10 @@ void nm_setting_option_set_boolean (NMSetting *setting, const char *opt_name, gboolean value); +NM_AVAILABLE_IN_1_26 +const char *const*nm_setting_option_get_all_names (NMSetting *setting, + guint *out_len); + /*****************************************************************************/ const GVariantType *nm_setting_get_dbus_property_type (NMSetting *setting, diff --git a/libnm/libnm.ver b/libnm/libnm.ver index b1abdc257d..5a3c5eeeac 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1733,6 +1733,7 @@ global: nm_setting_match_remove_kernel_command_line; nm_setting_match_remove_kernel_command_line_by_value; nm_setting_option_get; + nm_setting_option_get_all_names; nm_setting_option_get_boolean; nm_setting_option_get_uint32; nm_setting_option_set; From 49db9d8d787a69ea4afb78761effbb00d3dc0c85 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:33 +0200 Subject: [PATCH 16/21] libnm: add nm_setting_option_clear_by_name() More general purpose API for generic options of settings. The predicate function is also nicely usable via bindings. One question is about the form of the predicate. In this case, it is convenient to pass nm_ethtool_optname_is_coalesce(). On the other hand, it's not very flexible as it does not accept a user data argument. Use NMUtilsPredicateStr here, which is not flexible but convenient for where it's used. --- libnm-core/nm-core-internal.h | 5 ---- libnm-core/nm-setting-ethtool.c | 15 +++++------ libnm-core/nm-setting.c | 47 ++++++++++++++++++++++----------- libnm-core/nm-setting.h | 5 ++++ libnm/libnm.ver | 1 + 5 files changed, 44 insertions(+), 29 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 9fe658c1c2..6d7661b916 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -310,8 +310,6 @@ gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue /*****************************************************************************/ -typedef gboolean (*NMSettingOptionFilterFcn)(const char *name); - GHashTable *_nm_setting_option_hash (NMSetting *setting, gboolean create_if_necessary); @@ -325,9 +323,6 @@ guint _nm_setting_option_get_all (NMSetting *setting, gboolean _nm_setting_option_clear (NMSetting *setting, const char *optname); -gboolean _nm_setting_option_clear_all (NMSetting *setting, - NMSettingOptionFilterFcn filter); - /*****************************************************************************/ guint nm_setting_ethtool_init_features (NMSettingEthtool *setting, diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 1be74cc9cb..ac0310da82 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -226,9 +226,8 @@ nm_setting_ethtool_clear_features (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (_nm_setting_option_clear_all (NM_SETTING (setting), - &nm_ethtool_optname_is_feature)) - _notify_attributes (setting); + nm_setting_option_clear_by_name (NM_SETTING (setting), + nm_ethtool_optname_is_feature); } guint @@ -369,9 +368,8 @@ nm_setting_ethtool_clear_coalesce_all (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (_nm_setting_option_clear_all (NM_SETTING (setting), - &nm_ethtool_optname_is_coalesce)) - _notify_attributes (setting); + nm_setting_option_clear_by_name (NM_SETTING (setting), + nm_ethtool_optname_is_coalesce); } /** @@ -468,9 +466,8 @@ nm_setting_ethtool_clear_ring_all (NMSettingEthtool *setting) { g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - if (_nm_setting_option_clear_all (NM_SETTING (setting), - &nm_ethtool_optname_is_ring)) - _notify_attributes (setting); + nm_setting_option_clear_by_name (NM_SETTING (setting), + nm_ethtool_optname_is_ring); } /*****************************************************************************/ diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index a99a46b0ea..dfb2bed958 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -2525,30 +2525,47 @@ _nm_setting_option_clear (NMSetting *setting, return g_hash_table_remove (ht, optname); } -gboolean -_nm_setting_option_clear_all (NMSetting *setting, - NMSettingOptionFilterFcn filter) +/** + * nm_setting_option_clear_by_name: + * @setting: the #NMSetting + * @predicate: (allow-none) (scope call): the predicate for which names + * should be clear. + * If the predicate returns %TRUE for an option name, the option + * gets removed. If %NULL, all options will be removed. + * + * Since: 1.26 + */ +void +nm_setting_option_clear_by_name (NMSetting *setting, + NMUtilsPredicateStr predicate) { - GHashTable *ht; - const char *name; + GHashTable *hash; GHashTableIter iter; + const char *name; gboolean changed = FALSE; - nm_assert (NM_IS_SETTING (setting)); + g_return_if_fail (NM_IS_SETTING (setting)); - ht = _nm_setting_option_hash (setting, FALSE); - if (!ht) - return FALSE; + hash = _nm_setting_option_hash (NM_SETTING (setting), FALSE); + if (!hash) + return; - g_hash_table_iter_init (&iter, ht); - while (g_hash_table_iter_next (&iter, (gpointer *) &name, NULL)) { - if (!filter || filter (name)) { - g_hash_table_iter_remove (&iter); - changed = TRUE; + if (!predicate) { + changed = (g_hash_table_size (hash) > 0); + if (changed) + g_hash_table_remove_all (hash); + } else { + g_hash_table_iter_init (&iter, hash); + while (g_hash_table_iter_next (&iter, (gpointer *) &name, NULL)) { + if (predicate (name)) { + g_hash_table_iter_remove (&iter); + changed = TRUE; + } } } - return changed; + if (changed) + _nm_setting_option_notify (setting, TRUE); } /*****************************************************************************/ diff --git a/libnm-core/nm-setting.h b/libnm-core/nm-setting.h index a3a732218e..01cd1503e8 100644 --- a/libnm-core/nm-setting.h +++ b/libnm-core/nm-setting.h @@ -366,6 +366,11 @@ NM_AVAILABLE_IN_1_26 const char *const*nm_setting_option_get_all_names (NMSetting *setting, guint *out_len); + +NM_AVAILABLE_IN_1_26 +void nm_setting_option_clear_by_name (NMSetting *setting, + NMUtilsPredicateStr predicate); + /*****************************************************************************/ const GVariantType *nm_setting_get_dbus_property_type (NMSetting *setting, diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 5a3c5eeeac..030a68c885 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1732,6 +1732,7 @@ global: nm_setting_match_remove_driver_by_value; nm_setting_match_remove_kernel_command_line; nm_setting_match_remove_kernel_command_line_by_value; + nm_setting_option_clear_by_name; nm_setting_option_get; nm_setting_option_get_all_names; nm_setting_option_get_boolean; From 614f5f5a883f74beacf2b242a2a39d4a3536e558 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:33 +0200 Subject: [PATCH 17/21] libnm: use nm_setting_option_*() API in NMSettingEthtool --- libnm-core/nm-setting-ethtool.c | 61 ++++++++++----------------------- 1 file changed, 18 insertions(+), 43 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index ac0310da82..fd749780ca 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -140,19 +140,16 @@ NMTernary nm_setting_ethtool_get_feature (NMSettingEthtool *setting, const char *optname) { - GVariant *v; + gboolean v; g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), NM_TERNARY_DEFAULT); g_return_val_if_fail (optname && nm_ethtool_optname_is_feature (optname), NM_TERNARY_DEFAULT); - v = nm_setting_option_get (NM_SETTING (setting), optname); - if ( v - && g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) { - return g_variant_get_boolean (v) - ? NM_TERNARY_TRUE - : NM_TERNARY_FALSE; - } - return NM_TERNARY_DEFAULT; + if (!nm_setting_option_get_boolean (NM_SETTING (setting), optname, &v)) + return NM_TERNARY_DEFAULT; + return v + ? NM_TERNARY_TRUE + : NM_TERNARY_FALSE; } /** @@ -174,43 +171,16 @@ nm_setting_ethtool_set_feature (NMSettingEthtool *setting, const char *optname, NMTernary value) { - GHashTable *hash; - GVariant *v; - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); g_return_if_fail (optname && nm_ethtool_optname_is_feature (optname)); g_return_if_fail (NM_IN_SET (value, NM_TERNARY_DEFAULT, NM_TERNARY_FALSE, NM_TERNARY_TRUE)); - hash = _nm_setting_option_hash (NM_SETTING (setting), - value != NM_TERNARY_DEFAULT); - - if (value == NM_TERNARY_DEFAULT) { - if (hash) { - if (g_hash_table_remove (hash, optname)) - _notify_attributes (setting); - } - return; - } - - v = g_hash_table_lookup (hash, optname); - if ( v - && g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) { - if (g_variant_get_boolean (v)) { - if (value == NM_TERNARY_TRUE) - return; - } else { - if (value == NM_TERNARY_FALSE) - return; - } - } - - v = g_variant_ref_sink (g_variant_new_boolean (value != NM_TERNARY_FALSE)); - g_hash_table_insert (hash, - g_strdup (optname), - v); - _notify_attributes (setting); + if (value == NM_TERNARY_DEFAULT) + nm_setting_option_set (NM_SETTING (setting), optname, NULL); + else + nm_setting_option_set_boolean (NM_SETTING (setting), optname, (value != NM_TERNARY_FALSE)); } /** @@ -491,11 +461,16 @@ const char ** nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, guint *out_length) { + const char *const*names; + guint len; + g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), NULL); - return nm_utils_strdict_get_keys (_nm_setting_option_hash (NM_SETTING (setting), FALSE), - TRUE, - out_length); + names = nm_setting_option_get_all_names (NM_SETTING (setting), &len); + NM_SET_OUT (out_length, len); + return len > 0 + ? nm_memdup (names, sizeof (names[0]) * (((gsize) len) + 1u)) + : NULL; } /*****************************************************************************/ From 0533ab3c795a97674ce0ba49f469eaf173cb47e2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:34 +0200 Subject: [PATCH 18/21] all: avoid (soon to be) deprecated API instead of nm_setting_option*() --- clients/common/nm-meta-setting-desc.c | 152 +++++++----------- libnm-core/tests/test-setting.c | 150 +++++++++++------ .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 38 ++--- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 11 +- 4 files changed, 176 insertions(+), 175 deletions(-) diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c index 2c3e0d534a..7951a2333c 100644 --- a/clients/common/nm-meta-setting-desc.c +++ b/clients/common/nm-meta-setting-desc.c @@ -4097,78 +4097,57 @@ _gobject_enum_pre_set_notify_fcn_wireless_security_wep_key_type (const NMMetaPro static gconstpointer _get_fcn_ethtool (ARGS_GET_FCN) { - char *return_str; - guint32 u32; NMEthtoolID ethtool_id = property_info->property_typ_data->subtype.ethtool.ethtool_id; + const char *s; + guint32 u32; + gboolean b; RETURN_UNSUPPORTED_GET_TYPE (); - if (nm_ethtool_id_is_coalesce (ethtool_id)) { - if (!nm_setting_ethtool_get_coalesce (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname, - &u32)) { + if ( nm_ethtool_id_is_coalesce (ethtool_id) + || nm_ethtool_id_is_ring (ethtool_id)) { + if (!nm_setting_option_get_uint32 (setting, + nm_ethtool_data[ethtool_id]->optname, + &u32)) { NM_SET_OUT (out_is_default, TRUE); return NULL; } - return_str = g_strdup_printf ("%"G_GUINT32_FORMAT, u32); - RETURN_STR_TO_FREE (return_str); - } else if (nm_ethtool_id_is_feature (ethtool_id)) { - const char *s; - NMTernary val; - - val = nm_setting_ethtool_get_feature (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname); - - if (val == NM_TERNARY_TRUE) - s = N_("on"); - else if (val == NM_TERNARY_FALSE) - s = N_("off"); - else { - s = NULL; - NM_SET_OUT (out_is_default, TRUE); - } - - if (s && get_type == NM_META_ACCESSOR_GET_TYPE_PRETTY) - s = gettext (s); - return s; - } else if (nm_ethtool_id_is_ring (ethtool_id)) { - if (!nm_setting_ethtool_get_ring (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname, - &u32)) { - NM_SET_OUT (out_is_default, TRUE); - return NULL; - } - - return_str = g_strdup_printf ("%"G_GUINT32_FORMAT, u32); - RETURN_STR_TO_FREE (return_str); + RETURN_STR_TO_FREE (nm_strdup_int (u32)); } - nm_assert_not_reached(); - return NULL; + + nm_assert (nm_ethtool_id_is_feature (ethtool_id)); + + if (!nm_setting_option_get_boolean (setting, + nm_ethtool_data[ethtool_id]->optname, + &b)) { + NM_SET_OUT (out_is_default, TRUE); + return NULL; + } + + s = b + ? N_("on") + : N_("off"); + if (get_type == NM_META_ACCESSOR_GET_TYPE_PRETTY) + s = gettext (s); + return s; } static gboolean _set_fcn_ethtool (ARGS_SET_FCN) { - gint64 i64; NMEthtoolID ethtool_id = property_info->property_typ_data->subtype.ethtool.ethtool_id; - NMEthtoolType ethtool_type = nm_ethtool_id_to_type (ethtool_id); + gs_free char *value_to_free = NULL; + gint64 i64; + gboolean b; - if (NM_IN_SET (ethtool_type, - NM_ETHTOOL_TYPE_COALESCE, - NM_ETHTOOL_TYPE_RING)) { - if (_SET_FCN_DO_RESET_DEFAULT (property_info, modifier, value)) { - if (ethtool_type == NM_ETHTOOL_TYPE_COALESCE) - nm_setting_ethtool_clear_coalesce (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname); - else - nm_setting_ethtool_clear_ring (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname); - return TRUE; - } + if (_SET_FCN_DO_RESET_DEFAULT (property_info, modifier, value)) + goto do_unset; + + if ( nm_ethtool_id_is_coalesce (ethtool_id) + || nm_ethtool_id_is_ring (ethtool_id)) { i64 = _nm_utils_ascii_str_to_int64 (value, 10, 0, G_MAXUINT32, -1); - if (i64 == -1) { g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, _("'%s' is out of range [%"G_GUINT32_FORMAT", %"G_GUINT32_FORMAT"]"), @@ -4176,53 +4155,38 @@ _set_fcn_ethtool (ARGS_SET_FCN) return FALSE; } - if (ethtool_type == NM_ETHTOOL_TYPE_COALESCE) - nm_setting_ethtool_set_coalesce (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname, - (guint32) i64); - else - nm_setting_ethtool_set_ring (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname, - (guint32) i64); + nm_setting_option_set_uint32 (setting, + nm_ethtool_data[ethtool_id]->optname, + i64); return TRUE; } - if (ethtool_type == NM_ETHTOOL_TYPE_FEATURE) { - gs_free char *value_to_free = NULL; - NMTernary val; + nm_assert (nm_ethtool_id_is_feature (ethtool_id)); - if (_SET_FCN_DO_RESET_DEFAULT (property_info, modifier, value)) { - val = NM_TERNARY_DEFAULT; - goto set; - } - - value = nm_strstrip_avoid_copy_a (300, value, &value_to_free); - - if (NM_IN_STRSET (value, "1", "yes", "true", "on")) - val = NM_TERNARY_TRUE; - else if (NM_IN_STRSET (value, "0", "no", "false", "off")) - val = NM_TERNARY_FALSE; - else if (NM_IN_STRSET (value, "", "ignore", "default")) - val = NM_TERNARY_DEFAULT; - else { - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, - _("'%s' is not valid; use 'on', 'off', or 'ignore'"), - value); - return FALSE; - } - -set: - nm_setting_ethtool_set_feature (NM_SETTING_ETHTOOL (setting), - nm_ethtool_data[ethtool_id]->optname, - val); - return TRUE; + value = nm_strstrip_avoid_copy_a (300, value, &value_to_free); + if (NM_IN_STRSET (value, "1", "yes", "true", "on")) + b = TRUE; + else if (NM_IN_STRSET (value, "0", "no", "false", "off")) + b = FALSE; + else if (NM_IN_STRSET (value, "", "ignore", "default")) + goto do_unset; + else { + g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, + _("'%s' is not valid; use 'on', 'off', or 'ignore'"), + value); + return FALSE; } - nm_assert_not_reached(); + nm_setting_option_set_boolean (setting, + nm_ethtool_data[ethtool_id]->optname, + b); + return TRUE; - g_set_error (error, NM_UTILS_ERROR, NM_UTILS_ERROR_SETTING_MISSING, - _("ethtool property not supported")); - return FALSE; +do_unset: + nm_setting_option_set (setting, + nm_ethtool_data[ethtool_id]->optname, + NULL); + return TRUE; } static const char *const* diff --git a/libnm-core/tests/test-setting.c b/libnm-core/tests/test-setting.c index d861fbcb33..c7d54cbbcf 100644 --- a/libnm-core/tests/test-setting.c +++ b/libnm-core/tests/test-setting.c @@ -1498,6 +1498,52 @@ test_team_setting (void) /*****************************************************************************/ +static void +_setting_ethtool_set_feature (NMSettingEthtool *s_ethtool, + const char *opt_name, + NMTernary value) +{ + g_assert (NM_IS_SETTING_ETHTOOL (s_ethtool)); + + if (nmtst_get_rand_bool ()) { + nm_setting_ethtool_set_feature (s_ethtool, opt_name, value); + return; + } + + if (value == NM_TERNARY_DEFAULT) { + nm_setting_option_set (NM_SETTING (s_ethtool), opt_name, NULL); + return; + } + + if (nmtst_get_rand_bool ()) + nm_setting_option_set_boolean (NM_SETTING (s_ethtool), opt_name, value); + else + nm_setting_option_set (NM_SETTING (s_ethtool), opt_name, g_variant_new_boolean (value)); +} + +static NMTernary +_setting_ethtool_get_feature (NMSettingEthtool *s_ethtool, + const char *opt_name) +{ + GVariant *v; + gboolean b; + + switch (nmtst_get_rand_uint32 () % 3) { + case 0: + return nm_setting_ethtool_get_feature (s_ethtool, opt_name); + case 1: + if (!nm_setting_option_get_boolean (NM_SETTING (s_ethtool), opt_name, &b)) + return NM_TERNARY_DEFAULT; + return b; + default: + v = nm_setting_option_get (NM_SETTING (s_ethtool), opt_name); + if ( !v + || !g_variant_is_of_type (v, G_VARIANT_TYPE_BOOLEAN)) + return NM_TERNARY_DEFAULT; + return g_variant_get_boolean (v); + } +} + static void test_ethtool_features (void) { @@ -1519,16 +1565,16 @@ test_ethtool_features (void) s_ethtool = NM_SETTING_ETHTOOL (nm_setting_ethtool_new ()); nm_connection_add_setting (con, NM_SETTING (s_ethtool)); - nm_setting_ethtool_set_feature (s_ethtool, - NM_ETHTOOL_OPTNAME_FEATURE_RX, - NM_TERNARY_TRUE); - nm_setting_ethtool_set_feature (s_ethtool, - NM_ETHTOOL_OPTNAME_FEATURE_LRO, - NM_TERNARY_FALSE); + _setting_ethtool_set_feature (s_ethtool, + NM_ETHTOOL_OPTNAME_FEATURE_RX, + NM_TERNARY_TRUE); + _setting_ethtool_set_feature (s_ethtool, + NM_ETHTOOL_OPTNAME_FEATURE_LRO, + NM_TERNARY_FALSE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); nmtst_connection_normalize (con); @@ -1539,9 +1585,9 @@ test_ethtool_features (void) s_ethtool2 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con2, NM_TYPE_SETTING_ETHTOOL)); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool2, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); nmtst_assert_connection_verifies_without_normalization (con2); @@ -1566,9 +1612,9 @@ test_ethtool_features (void) s_ethtool3 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con3, NM_TYPE_SETTING_ETHTOOL)); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); - g_assert_cmpint (nm_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_RX), ==, NM_TERNARY_TRUE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_LRO), ==, NM_TERNARY_FALSE); + g_assert_cmpint (_setting_ethtool_get_feature (s_ethtool3, NM_ETHTOOL_OPTNAME_FEATURE_SG), ==, NM_TERNARY_DEFAULT); } static void @@ -1584,7 +1630,7 @@ test_ethtool_coalesce (void) NMSettingEthtool *s_ethtool; NMSettingEthtool *s_ethtool2; NMSettingEthtool *s_ethtool3; - guint32 out_value; + guint32 u32; con = nmtst_create_minimal_connection ("ethtool-coalesce", NULL, @@ -1593,12 +1639,12 @@ test_ethtool_coalesce (void) s_ethtool = NM_SETTING_ETHTOOL (nm_setting_ethtool_new ()); nm_connection_add_setting (con, NM_SETTING (s_ethtool)); - nm_setting_ethtool_set_coalesce (s_ethtool, - NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, - 4); + nm_setting_option_set_uint32 (NM_SETTING (s_ethtool), + NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, + 4); - g_assert_true (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &out_value)); - g_assert_cmpuint (out_value, ==, 4); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &u32)); + g_assert_cmpuint (u32, ==, 4); nmtst_connection_normalize (con); @@ -1609,8 +1655,8 @@ test_ethtool_coalesce (void) s_ethtool2 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con2, NM_TYPE_SETTING_ETHTOOL)); - g_assert_true (nm_setting_ethtool_get_coalesce (s_ethtool2, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &out_value)); - g_assert_cmpuint (out_value, ==, 4); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool2), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &u32)); + g_assert_cmpuint (u32, ==, 4); nmtst_assert_connection_verifies_without_normalization (con2); @@ -1635,24 +1681,24 @@ test_ethtool_coalesce (void) s_ethtool3 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con3, NM_TYPE_SETTING_ETHTOOL)); - g_assert_true (nm_setting_ethtool_get_coalesce (s_ethtool3, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &out_value)); - g_assert_cmpuint (out_value, ==, 4); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool3), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, &u32)); + g_assert_cmpuint (u32, ==, 4); - nm_setting_ethtool_clear_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES); - g_assert_false (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, NULL)); + nm_setting_option_set (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, NULL); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, NULL)); - nm_setting_ethtool_set_coalesce (s_ethtool, - NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, - 8); + nm_setting_option_set_uint32 (NM_SETTING (s_ethtool), + NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, + 8); - g_assert_true (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, &out_value)); - g_assert_cmpuint (out_value, ==, 8); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, &u32)); + g_assert_cmpuint (u32, ==, 8); - nm_setting_ethtool_clear_coalesce_all (s_ethtool); - g_assert_false (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, NULL)); - g_assert_false (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, NULL)); - g_assert_false (nm_setting_ethtool_get_coalesce (s_ethtool, NM_ETHTOOL_OPTNAME_COALESCE_TX_USECS, NULL)); + nm_setting_option_clear_by_name (NM_SETTING (s_ethtool), nm_ethtool_optname_is_coalesce); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_RX_FRAMES, NULL)); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_TX_FRAMES, NULL)); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_COALESCE_TX_USECS, NULL)); } static void @@ -1677,11 +1723,11 @@ test_ethtool_ring (void) s_ethtool = NM_SETTING_ETHTOOL (nm_setting_ethtool_new ()); nm_connection_add_setting (con, NM_SETTING (s_ethtool)); - nm_setting_ethtool_set_ring (s_ethtool, - NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, - 4); + nm_setting_option_set_uint32 (NM_SETTING (s_ethtool), + NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, + 4); - g_assert_true (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); g_assert_cmpuint (out_value, ==, 4); nmtst_connection_normalize (con); @@ -1693,7 +1739,7 @@ test_ethtool_ring (void) s_ethtool2 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con2, NM_TYPE_SETTING_ETHTOOL)); - g_assert_true (nm_setting_ethtool_get_ring (s_ethtool2, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool2), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); g_assert_cmpuint (out_value, ==, 4); nmtst_assert_connection_verifies_without_normalization (con2); @@ -1719,24 +1765,24 @@ test_ethtool_ring (void) s_ethtool3 = NM_SETTING_ETHTOOL (nm_connection_get_setting (con3, NM_TYPE_SETTING_ETHTOOL)); - g_assert_true (nm_setting_ethtool_get_ring (s_ethtool3, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool3), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); g_assert_cmpuint (out_value, ==, 4); - nm_setting_ethtool_clear_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO); - g_assert_false (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, NULL)); + nm_setting_option_set (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, NULL); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, NULL)); - nm_setting_ethtool_set_ring (s_ethtool, - NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, - 8); + nm_setting_option_set_uint32 (NM_SETTING (s_ethtool), + NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, + 8); - g_assert_true (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); + g_assert_true (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, &out_value)); g_assert_cmpuint (out_value, ==, 8); - nm_setting_ethtool_clear_ring_all (s_ethtool); - g_assert_false (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, NULL)); - g_assert_false (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_RX, NULL)); - g_assert_false (nm_setting_ethtool_get_ring (s_ethtool, NM_ETHTOOL_OPTNAME_RING_TX, NULL)); + nm_setting_option_clear_by_name (NM_SETTING (s_ethtool), nm_ethtool_optname_is_ring); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX_JUMBO, NULL)); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_RX, NULL)); + g_assert_false (nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), NM_ETHTOOL_OPTNAME_RING_TX, NULL)); } /*****************************************************************************/ 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 b0befd88da..80be40e785 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -4406,42 +4406,34 @@ parse_ethtool_option (const char *value, /* skip ethtool type && interface name */ w_iter = &words[2]; - if (ethtool_type == NM_ETHTOOL_TYPE_FEATURE) { - while (w_iter && *w_iter) { + while (w_iter && *w_iter) { + if (ethtool_type == NM_ETHTOOL_TYPE_FEATURE) { w_iter = _next_ethtool_options_nmternary (w_iter, ethtool_type, &ifcfg_option); - if (ifcfg_option.has_value) - nm_setting_ethtool_set_feature (*out_s_ethtool, - ifcfg_option.optname, - ifcfg_option.v.nmternary); + if (ifcfg_option.has_value) { + nm_setting_option_set_boolean (NM_SETTING (*out_s_ethtool), + ifcfg_option.optname, + ifcfg_option.v.nmternary != NM_TERNARY_FALSE); + } } - return; - } - if (NM_IN_SET (ethtool_type, - NM_ETHTOOL_TYPE_COALESCE, - NM_ETHTOOL_TYPE_RING)) { - while (w_iter && *w_iter) { + if (NM_IN_SET (ethtool_type, + NM_ETHTOOL_TYPE_COALESCE, + NM_ETHTOOL_TYPE_RING)) { w_iter = _next_ethtool_options_uint32 (w_iter, ethtool_type, &ifcfg_option); if (ifcfg_option.has_value) { - if (ethtool_type == NM_ETHTOOL_TYPE_COALESCE) - nm_setting_ethtool_set_coalesce (*out_s_ethtool, - ifcfg_option.optname, - ifcfg_option.v.u32); - else - nm_setting_ethtool_set_ring (*out_s_ethtool, - ifcfg_option.optname, - ifcfg_option.v.u32); + nm_setting_option_set_uint32 (NM_SETTING (*out_s_ethtool), + ifcfg_option.optname, + ifcfg_option.v.u32); } } - return; } - /* unsupported ethtool type */ - nm_assert_not_reached(); + + return; } /* /sbin/ethtool -s ${REALDEVICE} $opts */ 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 4bac775bb6..f116ace7b4 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1184,7 +1184,7 @@ write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro const char *iface; gboolean is_first; guint32 u32; - NMTernary t; + gboolean b; s_con = nm_connection_get_setting_connection (connection); if (s_con) { @@ -1204,20 +1204,19 @@ write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_FEATURE_FIRST; ethtool_id <= _NM_ETHTOOL_ID_FEATURE_LAST; ethtool_id++) { nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - t = nm_setting_ethtool_get_feature (s_ethtool, nm_ethtool_data[ethtool_id]->optname); - if (t == NM_TERNARY_DEFAULT) + if (!nm_setting_option_get_boolean (NM_SETTING (s_ethtool), nm_ethtool_data[ethtool_id]->optname, &b)) continue; _ethtool_gstring_prepare (&str, &is_first, 'K', iface); g_string_append_c (str, ' '); g_string_append (str, nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - g_string_append (str, t != NM_TERNARY_FALSE ? " on" : " off"); + g_string_append (str, b ? " on" : " off"); } is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_COALESCE_FIRST; ethtool_id <= _NM_ETHTOOL_ID_COALESCE_LAST; ethtool_id++) { nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - if (!nm_setting_ethtool_get_coalesce (s_ethtool, nm_ethtool_data[ethtool_id]->optname, &u32)) + if (!nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), nm_ethtool_data[ethtool_id]->optname, &u32)) continue; _ethtool_gstring_prepare (&str, &is_first, 'C', iface); @@ -1229,7 +1228,7 @@ write_ethtool_setting (NMConnection *connection, shvarFile *ifcfg, GError **erro is_first = TRUE; for (ethtool_id = _NM_ETHTOOL_ID_RING_FIRST; ethtool_id <= _NM_ETHTOOL_ID_RING_LAST; ethtool_id++) { nm_assert (nms_ifcfg_rh_utils_get_ethtool_name (ethtool_id)); - if (!nm_setting_ethtool_get_ring (s_ethtool, nm_ethtool_data[ethtool_id]->optname, &u32)) + if (!nm_setting_option_get_uint32 (NM_SETTING (s_ethtool), nm_ethtool_data[ethtool_id]->optname, &u32)) continue; _ethtool_gstring_prepare (&str, &is_first, 'G', iface); From 482f9c574edf1e5ac37dc1bb38b9a24e810ab98b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:34 +0200 Subject: [PATCH 19/21] libnm: deprecated nm_setting_ethtool_*_feature() API These are just aliases for the more general nm_setting_option_*() API. --- libnm-core/nm-setting-ethtool.c | 8 ++++++++ libnm-core/nm-setting-ethtool.h | 12 ++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index fd749780ca..0057e10e0c 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -135,6 +135,8 @@ _notify_attributes (NMSettingEthtool *self) * is enabled, disabled, or left untouched. * * Since: 1.14 + * + * Deprecated: 1.26: use nm_setting_option_get_boolean() instead. */ NMTernary nm_setting_ethtool_get_feature (NMSettingEthtool *setting, @@ -165,6 +167,8 @@ nm_setting_ethtool_get_feature (NMSettingEthtool *setting, * nm_ethtool_optname_is_feature(). * * Since: 1.14 + * + * Deprecated: 1.26: use nm_setting_option_set() or nm_setting_option_set_boolean() instead. */ void nm_setting_ethtool_set_feature (NMSettingEthtool *setting, @@ -190,6 +194,8 @@ nm_setting_ethtool_set_feature (NMSettingEthtool *setting, * Clears all offload features settings * * Since: 1.14 + * + * Deprecated: 1.26: use nm_setting_option_clear_by_name() with nm_ethtool_optname_is_feature() predicate instead. */ void nm_setting_ethtool_clear_features (NMSettingEthtool *setting) @@ -456,6 +462,8 @@ nm_setting_ethtool_clear_ring_all (NMSettingEthtool *setting) * @setting and may get invalidated when @setting gets modified. * * Since: 1.20 + * + * Deprecated: 1.26: use nm_setting_option_get_all_names() instead. */ const char ** nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, diff --git a/libnm-core/nm-setting-ethtool.h b/libnm-core/nm-setting-ethtool.h index 795b3a38dc..6bb8edc87a 100644 --- a/libnm-core/nm-setting-ethtool.h +++ b/libnm-core/nm-setting-ethtool.h @@ -129,20 +129,24 @@ NMSetting *nm_setting_ethtool_new (void); /*****************************************************************************/ +NM_AVAILABLE_IN_1_20 +NM_DEPRECATED_IN_1_26 +const char ** nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, + guint *out_length); + NM_AVAILABLE_IN_1_14 +NM_DEPRECATED_IN_1_26 NMTernary nm_setting_ethtool_get_feature (NMSettingEthtool *setting, const char *optname); NM_AVAILABLE_IN_1_14 +NM_DEPRECATED_IN_1_26 void nm_setting_ethtool_set_feature (NMSettingEthtool *setting, const char *optname, NMTernary value); NM_AVAILABLE_IN_1_14 +NM_DEPRECATED_IN_1_26 void nm_setting_ethtool_clear_features (NMSettingEthtool *setting); -NM_AVAILABLE_IN_1_20 -const char ** nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, - guint *out_length); - NM_AVAILABLE_IN_1_26 gboolean nm_setting_ethtool_get_coalesce (NMSettingEthtool *setting, const char *optname, From 280600f0be708c02aab55de720ce017e50264116 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 09:16:34 +0200 Subject: [PATCH 20/21] libnm: remove redundant nm_setting_ethtool_*_coalesce() API Note that nm_setting_ethtool_set_coalesce() used to coerce "coalesce-adaptive-[rt]x" values to 0 or 1. The alternative API doesn't do that. But so does nm_setting_option_set() not tell you whether the value you set is valid. That is not the options of the setters, for that we have verify(). --- libnm-core/nm-setting-ethtool.c | 209 -------------------------------- libnm-core/nm-setting-ethtool.h | 34 ------ libnm/libnm.ver | 8 -- 3 files changed, 251 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 0057e10e0c..28307e4941 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -112,14 +112,6 @@ G_DEFINE_TYPE (NMSettingEthtool, nm_setting_ethtool, NM_TYPE_SETTING) /*****************************************************************************/ -static void -_notify_attributes (NMSettingEthtool *self) -{ - _nm_setting_option_notify (NM_SETTING (self), TRUE); -} - -/*****************************************************************************/ - /** * nm_setting_ethtool_get_feature: * @setting: the #NMSettingEthtool @@ -245,207 +237,6 @@ nm_setting_ethtool_init_features (NMSettingEthtool *setting, return n_req; } -/** - * nm_setting_ethtool_get_coalesce: - * @setting: the #NMSettingEthtool - * @optname: option name of the coalescing setting to get - * @out_value (out) (allow-none): value of the coalescing setting - * - * Gets the value of coalescing setting. - * - * Note that @optname must be a valid name for a setting, according to - * nm_ethtool_optname_is_coalesce(). - * - * - * Returns: %TRUE and places the coalesce setting value in @out_value or %FALSE if unset. - * - * Since: 1.26 - */ -gboolean -nm_setting_ethtool_get_coalesce (NMSettingEthtool *setting, - const char *optname, - guint32 *out_value) -{ - g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); - g_return_val_if_fail (nm_ethtool_optname_is_coalesce (optname), FALSE); - - return nm_setting_option_get_uint32 (NM_SETTING (setting), - optname, - out_value); -} - -/** - * nm_setting_ethtool_set_coalesce: - * @setting: the #NMSettingEthtool - * @optname: option name of the coalesce setting - * @value: the new value to set. - * - * Sets a coalesce setting. - * - * Note that @optname must be a valid name for a coalesce setting, according to - * nm_ethtool_optname_is_coalesce(). - * - * Since: 1.26 - */ -void -nm_setting_ethtool_set_coalesce (NMSettingEthtool *setting, - const char *optname, - guint32 value) -{ - NMEthtoolID ethtool_id; - - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - - ethtool_id = nm_ethtool_id_get_by_name (optname); - - g_return_if_fail (nm_ethtool_id_is_coalesce (ethtool_id)); - - if (NM_IN_SET (ethtool_id, - NM_ETHTOOL_ID_COALESCE_ADAPTIVE_RX, - NM_ETHTOOL_ID_COALESCE_ADAPTIVE_TX)) - value = !!value; - - nm_setting_option_set_uint32 (NM_SETTING (setting), - optname, - value); - _notify_attributes (setting); -} - -/** - * nm_setting_ethtool_clear_coalesce: - * @setting: the #NMSettingEthtool - * @optname: option name of the coalesce setting - * - * Clear a coalesce setting - * - * Since: 1.26 - */ -void -nm_setting_ethtool_clear_coalesce (NMSettingEthtool *setting, - const char *optname) -{ - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - g_return_if_fail (nm_str_not_empty (optname)); - - if (_nm_setting_option_clear (NM_SETTING (setting), optname)) - _notify_attributes (setting); -} - -/** - * nm_setting_ethtool_clear_coalesce_all: - * @setting: the #NMSettingEthtool - * - * Clears all coalesce settings - * - * Since: 1.26 - */ -void -nm_setting_ethtool_clear_coalesce_all (NMSettingEthtool *setting) -{ - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - - nm_setting_option_clear_by_name (NM_SETTING (setting), - nm_ethtool_optname_is_coalesce); -} - -/** - * nm_setting_ethtool_get_ring: - * @setting: the #NMSettingEthtool - * @optname: option name of the ring setting to get - * @out_value (out) (allow-none): value of the ring setting - * - * Gets the value of ring setting. - * - * Note that @optname must be a valid name for a setting, according to - * nm_ethtool_optname_is_ring(). - * - * - * Returns: %TRUE and places the ring setting value in @out_value or %FALSE if unset. - * - * Since: 1.26 - */ -gboolean -nm_setting_ethtool_get_ring (NMSettingEthtool *setting, - const char *optname, - guint32 *out_value) -{ - g_return_val_if_fail (NM_IS_SETTING_ETHTOOL (setting), FALSE); - g_return_val_if_fail (nm_ethtool_optname_is_ring (optname), FALSE); - - return nm_setting_option_get_uint32 (NM_SETTING (setting), - optname, - out_value); -} - -/** - * nm_setting_ethtool_set_ring: - * @setting: the #NMSettingEthtool - * @optname: option name of the ring setting - * @value: the new value to set. - * - * Sets a ring setting. - * - * Note that @optname must be a valid name for a ring setting, according to - * nm_ethtool_optname_is_ring(). - * - * Since: 1.26 - */ -void -nm_setting_ethtool_set_ring (NMSettingEthtool *setting, - const char *optname, - guint32 value) -{ - NMEthtoolID ethtool_id; - - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - - ethtool_id = nm_ethtool_id_get_by_name (optname); - - g_return_if_fail (nm_ethtool_id_is_ring (ethtool_id)); - - nm_setting_option_set_uint32 (NM_SETTING (setting), - optname, - value); - _notify_attributes (setting); -} - -/** - * nm_setting_ethtool_clear_ring: - * @setting: the #NMSettingEthtool - * @optname: option name of the ring setting - * - * Clear a ring setting - * - * Since: 1.26 - */ -void -nm_setting_ethtool_clear_ring (NMSettingEthtool *setting, - const char *optname) -{ - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - g_return_if_fail (nm_str_not_empty (optname)); - - if (_nm_setting_option_clear (NM_SETTING (setting), optname)) - _notify_attributes (setting); -} - -/** - * nm_setting_ethtool_clear_ring_all: - * @setting: the #NMSettingEthtool - * - * Clears all ring settings - * - * Since: 1.26 - */ -void -nm_setting_ethtool_clear_ring_all (NMSettingEthtool *setting) -{ - g_return_if_fail (NM_IS_SETTING_ETHTOOL (setting)); - - nm_setting_option_clear_by_name (NM_SETTING (setting), - nm_ethtool_optname_is_ring); -} - /*****************************************************************************/ /** diff --git a/libnm-core/nm-setting-ethtool.h b/libnm-core/nm-setting-ethtool.h index 6bb8edc87a..27cb965826 100644 --- a/libnm-core/nm-setting-ethtool.h +++ b/libnm-core/nm-setting-ethtool.h @@ -147,40 +147,6 @@ NM_AVAILABLE_IN_1_14 NM_DEPRECATED_IN_1_26 void nm_setting_ethtool_clear_features (NMSettingEthtool *setting); -NM_AVAILABLE_IN_1_26 -gboolean nm_setting_ethtool_get_coalesce (NMSettingEthtool *setting, - const char *optname, - guint32 *out_value); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_set_coalesce (NMSettingEthtool *setting, - const char *optname, - guint32 value); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_clear_coalesce (NMSettingEthtool *setting, - const char *optname); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_clear_coalesce_all (NMSettingEthtool *setting); - -NM_AVAILABLE_IN_1_26 -gboolean nm_setting_ethtool_get_ring (NMSettingEthtool *setting, - const char *optname, - guint32 *out_value); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_set_ring (NMSettingEthtool *setting, - const char *optname, - guint32 value); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_clear_ring (NMSettingEthtool *setting, - const char *optname); - -NM_AVAILABLE_IN_1_26 -void nm_setting_ethtool_clear_ring_all (NMSettingEthtool *setting); - G_END_DECLS #endif /* __NM_SETTING_ETHTOOL_H__ */ diff --git a/libnm/libnm.ver b/libnm/libnm.ver index 030a68c885..dac75f2f91 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1710,14 +1710,6 @@ global: nm_setting_bridge_get_multicast_startup_query_count; nm_setting_bridge_get_multicast_startup_query_interval; nm_setting_connection_get_mud_url; - nm_setting_ethtool_clear_coalesce; - nm_setting_ethtool_clear_coalesce_all; - nm_setting_ethtool_clear_ring; - nm_setting_ethtool_clear_ring_all; - nm_setting_ethtool_get_coalesce; - nm_setting_ethtool_get_ring; - nm_setting_ethtool_set_coalesce; - nm_setting_ethtool_set_ring; nm_setting_match_add_driver; nm_setting_match_add_kernel_command_line; nm_setting_match_clear_drivers; From 16c8555b24417712d4e09da6d12938acc3734762 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 14 May 2020 20:57:53 +0200 Subject: [PATCH 21/21] libnm: check options in NMSettingEthtool.verify() in defined order Iterating the hash gives the entries in undefined order. That means, when validation would fail for more than one option, then the error message arbitrarily points out one or the other. Instead, process the entries in a defined order. --- libnm-core/nm-setting-ethtool.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/libnm-core/nm-setting-ethtool.c b/libnm-core/nm-setting-ethtool.c index 28307e4941..17fbdc62a1 100644 --- a/libnm-core/nm-setting-ethtool.c +++ b/libnm-core/nm-setting-ethtool.c @@ -198,6 +198,8 @@ nm_setting_ethtool_clear_features (NMSettingEthtool *setting) nm_ethtool_optname_is_feature); } +/*****************************************************************************/ + guint nm_setting_ethtool_init_features (NMSettingEthtool *setting, NMTernary *requested /* indexed by NMEthtoolID - _NM_ETHTOOL_ID_FEATURE_FIRST */) @@ -277,17 +279,16 @@ nm_setting_ethtool_get_optnames (NMSettingEthtool *setting, static gboolean verify (NMSetting *setting, NMConnection *connection, GError **error) { - GHashTable *hash; - GHashTableIter iter; - const char *optname; - GVariant *variant; + const char *const*optnames; + GVariant *const*variants; + guint len; + guint i; - hash = _nm_setting_option_hash (setting, FALSE); - if (!hash) - return TRUE; + len = _nm_setting_option_get_all (setting, &optnames, &variants); - g_hash_table_iter_init (&iter, hash); - while (g_hash_table_iter_next (&iter, (gpointer *) &optname, (gpointer *) &variant)) { + for (i = 0; i < len; i++) { + const char *optname = optnames[i]; + GVariant *variant = variants[i]; const GVariantType *variant_type; NMEthtoolID ethtool_id;