From 652de3b8d21af459819812fc9c80330191a191b5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Mar 2020 16:58:20 +0100 Subject: [PATCH 1/9] cli: use nm_utils_bin2hexstr_full() in ssid_to_hex() We already have an implementation for converting a binary array to hex. And, it doesn't require a GString for constructing the output that has an known length. --- clients/cli/utils.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/clients/cli/utils.c b/clients/cli/utils.c index 2e3a38f735..64a7cde07e 100644 --- a/clients/cli/utils.c +++ b/clients/cli/utils.c @@ -321,19 +321,14 @@ nmc_parse_args (nmc_arg_t *arg_arr, gboolean last, int *argc, char ***argv, GErr char * ssid_to_hex (const char *str, gsize len) { - GString *printable; - char *printable_str; - int i; - - if (str == NULL || len == 0) + if (len == 0) return NULL; - printable = g_string_new (NULL); - for (i = 0; i < len; i++) { - g_string_append_printf (printable, "%02X", (unsigned char) str[i]); - } - printable_str = g_string_free (printable, FALSE); - return printable_str; + return nm_utils_bin2hexstr_full (str, + len, + '\0', + TRUE, + NULL); } /* From ca4b5307426bc93e4df03aca62fc8182de27765b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Mar 2020 17:08:08 +0100 Subject: [PATCH 2/9] cli: use NM_CMP*() macros for compare_aps() in "clients/cli/devices.c" The compare pattern seems simple, but seems error prone and subtle. NM_CMP*() avoids that. For example, nm_access_point_get_strength() returns an uint8_t. C will promote those values to "int" before doing the subtraction. Likewise, nm_access_point_get_frequency() returns a uint32_t. This gets promoted to unsigned int when doing the subtraction. Afterwards, that is converted to a signed int. So both cases were in fact correct. But such things are not obvious. Also, as fallback sort by D-Bus path. While that is not semantically useful, we should use a defined sort order. --- clients/cli/devices.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 964b7dce83..302ab0f18a 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -1101,17 +1101,16 @@ compare_aps (gconstpointer a, gconstpointer b, gpointer user_data) { NMAccessPoint *apa = *(NMAccessPoint **)a; NMAccessPoint *apb = *(NMAccessPoint **)b; - int cmp; - cmp = nm_access_point_get_strength (apb) - nm_access_point_get_strength (apa); - if (cmp != 0) - return cmp; + NM_CMP_DIRECT (nm_access_point_get_strength (apb), nm_access_point_get_strength (apa)); + NM_CMP_DIRECT (nm_access_point_get_frequency (apa), nm_access_point_get_frequency (apb)); + NM_CMP_DIRECT (nm_access_point_get_max_bitrate (apb), nm_access_point_get_max_bitrate (apa)); - cmp = nm_access_point_get_frequency (apa) - nm_access_point_get_frequency (apb); - if (cmp != 0) - return cmp; + /* as fallback, just give it some stable order and use the D-Bus path (literally). */ + NM_CMP_DIRECT_STRCMP0 (nm_object_get_path (NM_OBJECT (apa)), + nm_object_get_path (NM_OBJECT (apb))); - return nm_access_point_get_max_bitrate (apb) - nm_access_point_get_max_bitrate (apa); + return 0; } static GPtrArray * From e0e39a745256204a2ac4727f84cc5d2ea684c70f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 9 Mar 2020 17:10:20 +0100 Subject: [PATCH 3/9] cli: take reference in sort_access_points() for "clients/cli/devices.c" It's not really necessary, but it feels slightly more correct. The only reason not to take a reference is to safe the overhead of increasing and decreasing the reference counter. But that doesn't matter for nmcli at this point (and is tiny anyway). Let the API make sure that the instances are kept alive. --- clients/cli/devices.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 302ab0f18a..5c18f7cf76 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -1117,13 +1117,14 @@ static GPtrArray * sort_access_points (const GPtrArray *aps) { GPtrArray *sorted; - int i; + guint i; g_return_val_if_fail (aps, NULL); sorted = g_ptr_array_sized_new (aps->len); + g_ptr_array_set_free_func (sorted, nm_g_object_unref); for (i = 0; i < aps->len; i++) - g_ptr_array_add (sorted, aps->pdata[i]); + g_ptr_array_add (sorted, g_object_ref (aps->pdata[i])); g_ptr_array_sort_with_data (sorted, compare_aps, NULL); return sorted; } @@ -1542,7 +1543,6 @@ show_device_info (NMDevice *device, NmCli *nmc) if ((NM_IS_DEVICE_WIFI (device))) { NMAccessPoint *active_ap = NULL; const char *active_bssid = NULL; - GPtrArray *aps; /* section AP */ if (!g_ascii_strcasecmp (nmc_fields_dev_show_sections[section_idx]->name, nmc_fields_dev_show_sections[4]->name)) { @@ -1560,6 +1560,7 @@ show_device_info (NMDevice *device, NmCli *nmc) g_ptr_array_add (out.output_data, arr); { + gs_unref_ptrarray GPtrArray *aps = NULL; APInfo info = { .nmc = nmc, .index = 1, @@ -1571,7 +1572,6 @@ show_device_info (NMDevice *device, NmCli *nmc) aps = sort_access_points (nm_device_wifi_get_access_points (NM_DEVICE_WIFI (device))); g_ptr_array_foreach (aps, fill_output_access_point, &info); - g_ptr_array_free (aps, FALSE); } print_data_prepare_width (out.output_data); @@ -2784,7 +2784,6 @@ show_access_point_info (NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) { NMAccessPoint *active_ap = NULL; const char *active_bssid = NULL; - GPtrArray *aps; NmcOutputField *arr; if (nm_device_get_state (NM_DEVICE (wifi)) == NM_DEVICE_STATE_ACTIVATED) { @@ -2797,6 +2796,7 @@ show_access_point_info (NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) g_ptr_array_add (out->output_data, arr); { + gs_unref_ptrarray GPtrArray *aps = NULL; APInfo info = { .nmc = nmc, .index = 1, @@ -2808,7 +2808,6 @@ show_access_point_info (NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) aps = sort_access_points (nm_device_wifi_get_access_points (wifi)); g_ptr_array_foreach (aps, fill_output_access_point, &info); - g_ptr_array_free (aps, TRUE); } print_data_prepare_width (out->output_data); From 30cf1885d4c3f6e348ce6a7d4189cad4c16e4c63 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Mar 2020 19:15:21 +0100 Subject: [PATCH 4/9] cli: cleanup selecting Wi-Fi device for `nmcli device wifi list` Refactor the selection of the Wi-Fi device by name. Avoid find_wifi_device_by_iface() to lookup by name. We already have a sorted list of candidate devices. The ifname is just an additional filter to exclude devices. So, we shouldn't use find_wifi_device_by_iface(), but instead check our prepared list of devices, whether it contains matching candidates. --- clients/cli/devices.c | 75 +++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 28 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 5c18f7cf76..548a381f29 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -3010,6 +3010,8 @@ do_device_wifi_list (NmCli *nmc, int argc, char **argv) NMDeviceWifi *wifi; ScanInfo *scan_info = NULL; WifiListData *data; + gboolean ifname_handled; + NMDevice *ifname_handled_candidate; guint i, j; devices = nmc_get_devices_sorted (nmc->client); @@ -3058,6 +3060,9 @@ do_device_wifi_list (NmCli *nmc, int argc, char **argv) } } + if (nmc->complete) + return nmc->return_value; + if (!nmc->required_fields || g_ascii_strcasecmp (nmc->required_fields, "common") == 0) fields_str = NMC_FIELDS_DEV_WIFI_LIST_COMMON; else if (!nmc->required_fields || g_ascii_strcasecmp (nmc->required_fields, "all") == 0) { @@ -3073,9 +3078,6 @@ do_device_wifi_list (NmCli *nmc, int argc, char **argv) return NMC_RESULT_ERROR_USER_INPUT; } - if (nmc->complete) - return nmc->return_value; - if (argc) { g_string_printf (nmc->return_text, _("Error: invalid extra argument '%s'."), *argv); return NMC_RESULT_ERROR_USER_INPUT; @@ -3092,38 +3094,55 @@ do_device_wifi_list (NmCli *nmc, int argc, char **argv) return NMC_RESULT_ERROR_USER_INPUT; } - if (ifname) { - device = find_wifi_device_by_iface (devices, ifname, NULL); - if (!device) { - g_string_printf (nmc->return_text, _("Error: Device '%s' not found."), ifname); - return NMC_RESULT_ERROR_NOT_FOUND; - } + ifname_handled = (ifname == NULL); + ifname_handled_candidate = NULL; - if (NM_IS_DEVICE_WIFI (device)) { - devices[0] = device; - devices[1] = NULL; - } else { - if ( nm_device_get_device_type (device) == NM_DEVICE_TYPE_GENERIC - && g_strcmp0 (nm_device_get_type_description (device), "wifi") == 0) { - g_string_printf (nmc->return_text, - _("Error: Device '%s' was not recognized as a Wi-Fi device, check NetworkManager Wi-Fi plugin."), - ifname); - } else { - g_string_printf (nmc->return_text, - _("Error: Device '%s' is not a Wi-Fi device."), - ifname); + j = 0; + for (i = 0; devices[i]; i++) { + const char *dev_iface; + + device = devices[i]; + dev_iface = nm_device_get_iface (device); + + if (ifname) { + if (!nm_streq0 (ifname, dev_iface)) + continue; + if (!NM_IS_DEVICE_WIFI (device)) { + if ( nm_device_get_device_type (device) == NM_DEVICE_TYPE_GENERIC + && nm_streq0 (nm_device_get_type_description (device), "wifi")) + ifname_handled_candidate = device; + else if (!ifname_handled_candidate) + ifname_handled_candidate = device; + continue; } - return NMC_RESULT_ERROR_UNKNOWN; + ifname_handled = TRUE; + } else { + if (!NM_IS_DEVICE_WIFI (device)) + continue; } - } - /* Filter out non-wifi devices */ - for (i = 0, j = 0; devices[i]; i++) { - if (NM_IS_DEVICE_WIFI (devices[i])) - devices[j++] = devices[i]; + devices[j++] = device; } devices[j] = NULL; + if (!ifname_handled) { + if (!ifname_handled_candidate) { + g_string_printf (nmc->return_text, + _("Error: Device '%s' not found."), + ifname); + } else if ( nm_device_get_device_type (ifname_handled_candidate) == NM_DEVICE_TYPE_GENERIC + && nm_streq0 (nm_device_get_type_description (ifname_handled_candidate), "wifi")) { + g_string_printf (nmc->return_text, + _("Error: Device '%s' was not recognized as a Wi-Fi device, check NetworkManager Wi-Fi plugin."), + ifname); + } else { + g_string_printf (nmc->return_text, + _("Error: Device '%s' is not a Wi-Fi device."), + ifname); + } + return NMC_RESULT_ERROR_NOT_FOUND; + } + /* Start a new scan for devices that need it */ for (i = 0; devices[i]; i++) { wifi = (NMDeviceWifi *) devices[i]; From 49dacaa34e69ab35f2f2cd508d7c4c52770e4319 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 10 Mar 2020 20:01:00 +0100 Subject: [PATCH 5/9] cli: use slice allocator in do_device_wifi_list() and designated initializers for data --- clients/cli/devices.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 548a381f29..8b58f08c5f 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -2921,14 +2921,14 @@ wifi_list_finish (WifiListData *data) g_signal_handler_disconnect (data->wifi, data->last_scan_id); nm_clear_g_source (&data->timeout_id); nm_clear_g_cancellable (&data->scan_cancellable); - g_slice_free (WifiListData, data); + nm_g_slice_free (data); if (info->nmc->should_wait == 0) { for (i = 0; info->devices[i]; i++) g_object_unref (info->devices[i]); g_free (info->devices); g_array_unref (info->out_indices); - g_free (info); + nm_g_slice_free (info); } } @@ -3149,25 +3149,36 @@ do_device_wifi_list (NmCli *nmc, int argc, char **argv) g_object_ref (wifi); if ( rescan_cutoff == 0 - || (rescan_cutoff > 0 && nm_device_wifi_get_last_scan (wifi) >= rescan_cutoff)) + || ( rescan_cutoff > 0 + && nm_device_wifi_get_last_scan (wifi) >= rescan_cutoff)) continue; if (!scan_info) { - scan_info = g_new0 (ScanInfo, 1); - scan_info->out_indices = g_array_ref (out_indices); - scan_info->tmpl = tmpl; - scan_info->bssid_user = bssid_user; - scan_info->nmc = nmc; + scan_info = g_slice_new (ScanInfo); + *scan_info = (ScanInfo) { + .out_indices = g_array_ref (out_indices), + .tmpl = tmpl, + .bssid_user = bssid_user, + .nmc = nmc, + }; } nmc->should_wait++; - data = g_slice_new0 (WifiListData); - data->wifi = wifi; - data->scan_info = scan_info; - data->last_scan_id = g_signal_connect (wifi, "notify::" NM_DEVICE_WIFI_LAST_SCAN, - G_CALLBACK (wifi_last_scan_updated), data); - data->scan_cancellable = g_cancellable_new (); - data->timeout_id = g_timeout_add_seconds (15, wifi_list_scan_timeout, data); + + data = g_slice_new (WifiListData); + *data = (WifiListData) { + .wifi = wifi, + .scan_info = scan_info, + .last_scan_id = g_signal_connect (wifi, + "notify::" NM_DEVICE_WIFI_LAST_SCAN, + G_CALLBACK (wifi_last_scan_updated), + data), + .scan_cancellable = g_cancellable_new (), + .timeout_id = g_timeout_add_seconds (15, + wifi_list_scan_timeout, + data), + }; + nm_device_wifi_request_scan_async (wifi, data->scan_cancellable, wifi_list_rescan_cb, data); } From a01355ba64ec7c1e56cbe5a1a6fd603c3e27820c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Mar 2020 13:10:04 +0100 Subject: [PATCH 6/9] cli: add get_type argument to ap_wpa_rsn_flags_to_string() for optional i18n Will be used later. --- clients/cli/devices.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 8b58f08c5f..464cbf3eb7 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -27,9 +27,9 @@ /*****************************************************************************/ static char * -ap_wpa_rsn_flags_to_string (NM80211ApSecurityFlags flags) +ap_wpa_rsn_flags_to_string (NM80211ApSecurityFlags flags, NMMetaAccessorGetType get_type) { - char *flags_str[14]; + char *flags_str[16]; int i = 0; if (flags & NM_802_11_AP_SEC_PAIR_WEP40) @@ -56,13 +56,17 @@ ap_wpa_rsn_flags_to_string (NM80211ApSecurityFlags flags) flags_str[i++] = "sae"; if (flags & NM_802_11_AP_SEC_KEY_MGMT_OWE) flags_str[i++] = "owe"; - /* Make sure you grow flags_str when adding items here. */ - if (i == 0) - flags_str[i++] = _("(none)"); + /* Make sure you grow flags_str when adding items here. */ + nm_assert (i < G_N_ELEMENTS (flags_str)); + + if (i == 0) { + if (get_type == NM_META_ACCESSOR_GET_TYPE_PRETTY) + return g_strdup (_("(none)")); + return g_strdup ("(none)"); + } flags_str[i] = NULL; - return g_strjoinv (" ", flags_str); } @@ -1189,8 +1193,8 @@ fill_output_access_point (gpointer data, gpointer user_data) freq_str = g_strdup_printf (_("%u MHz"), freq); bitrate_str = g_strdup_printf (_("%u Mbit/s"), bitrate/1000); strength_str = g_strdup_printf ("%u", strength); - wpa_flags_str = ap_wpa_rsn_flags_to_string (wpa_flags); - rsn_flags_str = ap_wpa_rsn_flags_to_string (rsn_flags); + wpa_flags_str = ap_wpa_rsn_flags_to_string (wpa_flags, NM_META_ACCESSOR_GET_TYPE_PRETTY); + rsn_flags_str = ap_wpa_rsn_flags_to_string (rsn_flags, NM_META_ACCESSOR_GET_TYPE_PRETTY); sig_bars = nmc_wifi_strength_bars (strength); security_str = g_string_new (NULL); From 3cc99c9f8c87a0d3ee29041fa7b18f6e55a3e996 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Mar 2020 14:39:27 +0100 Subject: [PATCH 7/9] cli: return typed PrintDataCol array from _output_selection_parse() It makes debugging and understanding the code slightly simpler, if we have a pointer of correct type, instead of returning a GArray. We don't need the GArray at this point anymore. --- clients/cli/utils.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/clients/cli/utils.c b/clients/cli/utils.c index 64a7cde07e..6dcb97b9fc 100644 --- a/clients/cli/utils.c +++ b/clients/cli/utils.c @@ -773,7 +773,8 @@ _output_selection_complete (GArray *cols) static gboolean _output_selection_parse (const NMMetaAbstractInfo *const*fields, const char *fields_str, - GArray **out_cols, + PrintDataCol **out_cols_data, + guint *out_cols_len, GPtrArray **out_gfree_keeper, GError **error) { @@ -807,7 +808,8 @@ _output_selection_parse (const NMMetaAbstractInfo *const*fields, _output_selection_complete (cols); - *out_cols = g_steal_pointer (&cols); + *out_cols_len = cols->len; + *out_cols_data = (PrintDataCol *) g_array_free (g_steal_pointer (&cols), FALSE); *out_gfree_keeper = g_steal_pointer (&gfree_keeper); return TRUE; } @@ -1374,20 +1376,24 @@ nmc_print (const NmcConfig *nmc_config, GError **error) { gs_unref_ptrarray GPtrArray *gfree_keeper = NULL; - gs_unref_array GArray *cols = NULL; + gs_free PrintDataCol *cols_data = NULL; + guint cols_len; gs_unref_array GArray *header_row = NULL; gs_unref_array GArray *cells = NULL; - if (!_output_selection_parse (fields, fields_str, - &cols, &gfree_keeper, + if (!_output_selection_parse (fields, + fields_str, + &cols_data, + &cols_len, + &gfree_keeper, error)) return FALSE; _print_fill (nmc_config, targets, targets_data, - &g_array_index (cols, PrintDataCol, 0), - cols->len, + cols_data, + cols_len, &header_row, &cells); From 5bef7d7453ae050698c888901f57563a8be6df77 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 13 Mar 2020 12:17:07 +0100 Subject: [PATCH 8/9] cli: minor cleanup dropping unnecessary local variables --- clients/cli/utils.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/clients/cli/utils.c b/clients/cli/utils.c index 6dcb97b9fc..d35ed26631 100644 --- a/clients/cli/utils.c +++ b/clients/cli/utils.c @@ -662,7 +662,6 @@ _output_selection_append (GArray *cols, guint i; const NMMetaAbstractInfo *const*nested; NMMetaSelectionResultList *selection; - const NMMetaSelectionItem *si; col_idx = cols->len; @@ -683,6 +682,8 @@ _output_selection_append (GArray *cols, gs_free char *allowed_fields = NULL; if (parent_idx != PRINT_DATA_COL_PARENT_NIL) { + const NMMetaSelectionItem *si; + si = g_array_index (cols, PrintDataCol, parent_idx).selection_item; allowed_fields = nm_meta_abstract_info_get_nested_names_str (si->info, si->self_selection); } @@ -714,10 +715,9 @@ _output_selection_append (GArray *cols, g_ptr_array_add (gfree_keeper, selection); for (i = 0; i < selection->num; i++) { - si = &selection->items[i]; if (!_output_selection_append (cols, col_idx, - si, + &selection->items[i], gfree_keeper, error)) return FALSE; @@ -799,10 +799,11 @@ _output_selection_parse (const NMMetaAbstractInfo *const*fields, cols = g_array_new (FALSE, TRUE, sizeof (PrintDataCol)); for (i = 0; i < selection->num; i++) { - const NMMetaSelectionItem *si = &selection->items[i]; - - if (!_output_selection_append (cols, PRINT_DATA_COL_PARENT_NIL, - si, gfree_keeper, error)) + if (!_output_selection_append (cols, + PRINT_DATA_COL_PARENT_NIL, + &selection->items[i], + gfree_keeper, + error)) return FALSE; } From 3d2b982fb758ffc781132b7ebec878b64ea94fa0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 13 Mar 2020 12:17:18 +0100 Subject: [PATCH 9/9] cli: fix out of bounds access in _print_fill() cols_len might be larger than header_row->len. That is when the cols has entries that are not leaf entries (which currently I think is never the case). Fix it to use the right variable for the length of the row. --- clients/cli/utils.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clients/cli/utils.c b/clients/cli/utils.c index d35ed26631..d8524c55a1 100644 --- a/clients/cli/utils.c +++ b/clients/cli/utils.c @@ -1140,7 +1140,8 @@ _print_fill (const NmcConfig *nmc_config, header_cell->width = nmc_string_screen_width (header_cell->title, NULL); for (i_row = 0; i_row < targets_len; i_row++) { - const PrintDataCell *cell = &g_array_index (cells, PrintDataCell, i_row * cols_len + i_col); + const PrintDataCell *cells_line = &g_array_index (cells, PrintDataCell, i_row * header_row->len); + const PrintDataCell *cell = &cells_line[i_col]; const char *const*i_strv; switch (cell->text_format) {