[PATCH v2 1/5] station: commonize the building of diagnostic dict

[PATCH v2 1/5] station: commonize the building of diagnostic dict

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3872 bytes --]

AP mode will use the same structure for its diagnostic interface
and mostly the same dictionary keys. Apart from ConnectedBss and
Address being different, the remainder are the same so the
netdev_station_info to DBus dictionary conversion has been made
common so both station and AP can use it to build its diagnostic
dictionaries.
---
 src/station.c | 71 ++++++++++++++++++++++++++++++---------------------
 src/station.h |  3 +++
 2 files changed, 45 insertions(+), 29 deletions(-)

v2:
 * Generalized the dictionary building so both station and AP could
   use it.

diff --git a/src/station.c b/src/station.c
index c65fc0cc..3582ea24 100644
--- a/src/station.c
+++ b/src/station.c
@@ -3448,36 +3448,16 @@ static void station_setup_interface(struct l_dbus_interface *interface)
 					station_property_get_state, NULL);
 }
 
-static void station_destroy_interface(void *user_data)
-{
-	struct station *station = user_data;
-
-	station_free(station);
-}
-
-static void station_get_diagnostic_cb(const struct netdev_station_info *info,
-					void *user_data)
+/*
+ * Appends values from netdev_station_info into a DBus dictionary. This assumes
+ * the DBus dictionary array has already been 'entered', and expects the
+ * caller to 'leave' once called. This does not append the station address
+ * since the dictionary key name may be different depending on the caller.
+ */
+bool station_info_to_dict(const struct netdev_station_info *info,
+				struct l_dbus_message_builder *builder)
 {
-	struct station *station = user_data;
-	struct l_dbus_message *reply;
-	struct l_dbus_message_builder *builder;
-	int16_t rssi;
-
-	if (!info) {
-		reply = dbus_error_aborted(station->get_station_pending);
-		goto done;
-	}
-
-	reply = l_dbus_message_new_method_return(station->get_station_pending);
-
-	rssi = (int16_t)info->cur_rssi;
-
-	builder = l_dbus_message_builder_new(reply);
-
-	l_dbus_message_builder_enter_array(builder, "{sv}");
-
-	dbus_append_dict_basic(builder, "ConnectedBss", 's',
-					util_address_to_string(info->addr));
+	int16_t rssi = (int16_t)info->cur_rssi;
 
 	if (info->have_cur_rssi)
 		dbus_append_dict_basic(builder, "RSSI", 'n', &rssi);
@@ -3544,6 +3524,39 @@ static void station_get_diagnostic_cb(const struct netdev_station_info *info,
 		dbus_append_dict_basic(builder, "ExpectedThroughput", 'u',
 					&info->expected_throughput);
 
+	return true;
+}
+
+static void station_destroy_interface(void *user_data)
+{
+	struct station *station = user_data;
+
+	station_free(station);
+}
+
+static void station_get_diagnostic_cb(const struct netdev_station_info *info,
+					void *user_data)
+{
+	struct station *station = user_data;
+	struct l_dbus_message *reply;
+	struct l_dbus_message_builder *builder;
+
+	if (!info) {
+		reply = dbus_error_aborted(station->get_station_pending);
+		goto done;
+	}
+
+	reply = l_dbus_message_new_method_return(station->get_station_pending);
+
+	builder = l_dbus_message_builder_new(reply);
+
+	l_dbus_message_builder_enter_array(builder, "{sv}");
+
+	dbus_append_dict_basic(builder, "ConnectedBss", 's',
+					util_address_to_string(info->addr));
+
+	station_info_to_dict(info, builder);
+
 	l_dbus_message_builder_leave_array(builder);
 	l_dbus_message_builder_finalize(builder);
 	l_dbus_message_builder_destroy(builder);
diff --git a/src/station.h b/src/station.h
index c4fd505b..f6b46bd1 100644
--- a/src/station.h
+++ b/src/station.h
@@ -100,3 +100,6 @@ void station_network_foreach(struct station *station,
 				void *user_data);
 struct l_queue *station_get_bss_list(struct station *station);
 struct scan_bss *station_get_connected_bss(struct station *station);
+
+bool station_info_to_dict(const struct netdev_station_info *info,
+				struct l_dbus_message_builder *builder);
-- 
2.26.2

[PATCH v2 2/5] ap: add AP diagnostic interface

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4731 bytes --]

This adds a new AccessPointDiagnostic interface. This interface
provides similar low level functionality as StationDiagnostic, but
for when IWD is in AP mode. This uses netdev_get_all_stations
which will dump all stations, parse, and return each station in
an individual callback. Once the dump is complete the destroy is
called and all data is packaged as an array of dictionaries.
---
 src/ap.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

v2:
 * Use station_info_to_dict
 * One glaring fault with this patch is that AP now includes
   station.h. This is not desireable but I was at a loss finding
   an existing module which already included dbus and would be
   appropriate for this utility. Maybe someone has suggestions.
   The only other option I could imagine would be creating a new
   utility module or including DBus in util.c/h

diff --git a/src/ap.c b/src/ap.c
index d9e7e404..fe812a33 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -52,6 +52,7 @@
 #include "src/eap-wsc.h"
 #include "src/ap.h"
 #include "src/storage.h"
+#include "src/station.h"
 
 struct ap_state {
 	struct netdev *netdev;
@@ -2963,6 +2964,89 @@ static void ap_destroy_interface(void *user_data)
 	l_free(ap_if);
 }
 
+struct diagnostic_data {
+	struct l_dbus_message *pending;
+	struct l_dbus_message_builder *builder;
+};
+
+static void ap_get_station_cb(const struct netdev_station_info *info,
+				void *user_data)
+{
+	struct diagnostic_data *data = user_data;
+
+	/* First station info */
+	if (!data->builder) {
+		struct l_dbus_message *reply =
+				l_dbus_message_new_method_return(data->pending);
+
+		data->builder = l_dbus_message_builder_new(reply);
+
+		l_dbus_message_builder_enter_array(data->builder, "a{sv}");
+	}
+
+	l_dbus_message_builder_enter_array(data->builder, "{sv}");
+	dbus_append_dict_basic(data->builder, "Address", 's',
+					util_address_to_string(info->addr));
+
+	station_info_to_dict(info, data->builder);
+
+	l_dbus_message_builder_leave_array(data->builder);
+}
+
+static void ap_get_station_destroy(void *user_data)
+{
+	struct diagnostic_data *data = user_data;
+	struct l_dbus_message *reply;
+
+	if (!data->builder) {
+		reply = l_dbus_message_new_method_return(data->pending);
+
+		data->builder = l_dbus_message_builder_new(reply);
+
+		l_dbus_message_builder_enter_array(data->builder, "a{sv}");
+	}
+
+	l_dbus_message_builder_leave_array(data->builder);
+	reply = l_dbus_message_builder_finalize(data->builder);
+	l_dbus_message_builder_destroy(data->builder);
+
+	dbus_pending_reply(&data->pending, reply);
+
+	l_free(data);
+}
+
+static struct l_dbus_message *ap_dbus_get_diagnostics(struct l_dbus *dbus,
+		struct l_dbus_message *message, void *user_data)
+{
+	struct ap_if_data *ap_if = user_data;
+	struct diagnostic_data *data;
+	int ret;
+
+	data = l_new(struct diagnostic_data, 1);
+	data->pending = l_dbus_message_ref(message);
+
+	ret = netdev_get_all_stations(ap_if->ap->netdev, ap_get_station_cb,
+					data, ap_get_station_destroy);
+
+	if (ret < 0) {
+		l_dbus_message_unref(data->pending);
+		l_free(data);
+		return dbus_error_from_errno(ret, message);
+	}
+
+	return NULL;
+}
+
+static void ap_setup_diagnostic_interface(struct l_dbus_interface *interface)
+{
+	l_dbus_interface_method(interface, "GetDiagnostics", 0,
+				ap_dbus_get_diagnostics, "aa{sv}", "", "diagnostic");
+}
+
+static void ap_diagnostic_interface_destroy(void *user_data)
+{
+}
+
 static void ap_add_interface(struct netdev *netdev)
 {
 	struct ap_if_data *ap_if;
@@ -2978,12 +3062,16 @@ static void ap_add_interface(struct netdev *netdev)
 	/* setup ap dbus interface */
 	l_dbus_object_add_interface(dbus_get_bus(),
 			netdev_get_path(netdev), IWD_AP_INTERFACE, ap_if);
+	l_dbus_object_add_interface(dbus_get_bus(), netdev_get_path(netdev),
+			IWD_AP_DIAGNOSTIC_INTERFACE, ap_if);
 }
 
 static void ap_remove_interface(struct netdev *netdev)
 {
 	l_dbus_object_remove_interface(dbus_get_bus(),
 			netdev_get_path(netdev), IWD_AP_INTERFACE);
+	l_dbus_object_remove_interface(dbus_get_bus(), netdev_get_path(netdev),
+			IWD_AP_DIAGNOSTIC_INTERFACE);
 }
 
 static void ap_netdev_watch(struct netdev *netdev,
@@ -3014,6 +3102,9 @@ static int ap_init(void)
 
 	l_dbus_register_interface(dbus_get_bus(), IWD_AP_INTERFACE,
 			ap_setup_interface, ap_destroy_interface, false);
+	l_dbus_register_interface(dbus_get_bus(), IWD_AP_DIAGNOSTIC_INTERFACE,
+			ap_setup_diagnostic_interface,
+			ap_diagnostic_interface_destroy, false);
 
 	/*
 	 * Reusing [General].EnableNetworkConfiguration as a switch to enable
-- 
2.26.2

[PATCH v2 3/5] client: implement display_dictionary

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4657 bytes --]

This takes a Dbus iterator which has been entered into a
dictionary and prints out each key and value. It requires
a mapping which maps keys to types and units. For simple
cases the mapping will consist of a dbus type character
and a units string, e.g. dBm, Kbit/s etc. For more complex
printing which requires processing the value the 'units'
void* cant be set to a function which can be custom written
to handle the value.
---
 client/display.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
 client/display.h | 22 ++++++++++++
 2 files changed, 116 insertions(+)

v2:
 * Generalized the printing of DBus dictionaries.

diff --git a/client/display.c b/client/display.c
index 88bc913f..d7baaac2 100644
--- a/client/display.c
+++ b/client/display.c
@@ -393,6 +393,100 @@ void display_command_line(const char *command_family,
 	l_free(cmd_line);
 }
 
+static const struct display_dict_mapping *find_mapping(const char *key,
+				const struct display_dict_mapping *mapping)
+{
+	int idx = 0;
+
+	while (mapping[idx].key) {
+		if (!strcmp(mapping[idx].key, key))
+			return &mapping[idx];
+
+		idx++;
+	}
+
+	return NULL;
+}
+
+void display_dictionary(struct l_dbus_message_iter *dict,
+			const struct display_dict_mapping *mapping,
+			const char *margin, int name_column_width,
+			int value_column_width)
+{
+	struct l_dbus_message_iter variant;
+	const char *key;
+	const struct display_dict_mapping *map;
+	display_dict_custom_func_t custom;
+	char display_text[160];
+
+	while (l_dbus_message_iter_next_entry(dict, &key, &variant)) {
+		const char *s_value;
+		uint32_t u_value;
+		int16_t n_value;
+		uint8_t y_value;
+
+		map = find_mapping(key, mapping);
+		if (!map)
+			continue;
+
+		switch (map->type) {
+		case 0:
+			if (!map->units)
+				continue;
+
+			custom = (display_dict_custom_func_t)map->units;
+
+			custom(&variant, key, margin, name_column_width,
+					value_column_width);
+
+			/* custom should handle any units, so continue */
+			continue;
+
+		case 's':
+			l_dbus_message_iter_get_variant(&variant, "s",
+							&s_value);
+			sprintf(display_text, "%s%-*s%-*s", margin,
+					name_column_width, key,
+					value_column_width, s_value);
+			break;
+
+		case 'u':
+			l_dbus_message_iter_get_variant(&variant, "u",
+							&u_value);
+			sprintf(display_text, "%s%-*s%-*u", margin,
+						name_column_width, key,
+						value_column_width, u_value);
+			break;
+
+		case 'n':
+			l_dbus_message_iter_get_variant(&variant, "n",
+							&n_value);
+			sprintf(display_text, "%s%-*s%-*i", margin,
+						name_column_width, key,
+						value_column_width, n_value);
+			break;
+
+		case 'y':
+			l_dbus_message_iter_get_variant(&variant, "y",
+							&y_value);
+			sprintf(display_text, "%s%-*s%-*u", margin,
+						name_column_width, key,
+						value_column_width, y_value);
+			break;
+
+		default:
+			display("type %c not handled", map->type);
+			continue;
+		}
+
+		if (map->units)
+			display("%s %s\n", display_text,
+					(const char *)map->units);
+		else
+			display("%s\n", display_text);
+	}
+}
+
 static void display_completion_matches(char **matches, int num_matches,
 								int max_length)
 {
diff --git a/client/display.h b/client/display.h
index b5df944a..78b486f5 100644
--- a/client/display.h
+++ b/client/display.h
@@ -22,6 +22,24 @@
 
 struct command;
 struct command_family;
+struct l_dbus_message_iter;
+
+typedef void (*display_dict_custom_func_t)(struct l_dbus_message_iter *variant,
+				const char *key, const char *margin,
+				int name_column_width, int value_column_width);
+
+/*
+ * Maps dictionary keys to types/units. 'type' should be a valid DBus type, or
+ * zero for displaying in a custom fashion. When the display needs to be
+ * customized 'units' should point to a custom display function of the form
+ * display_dict_custom_func_t which should display the entire value as well
+ * as any units required.
+ */
+struct display_dict_mapping {
+	const char *key;
+	char type;
+	void *units;
+};
 
 #define COLOR_BOLDGRAY	"\x1B[1;30m"
 #define COLOR_GRAY	"\x1b[37m"
@@ -41,6 +59,10 @@ void display_table_footer(void);
 void display_error(const char *error);
 void display_command_line(const char *command_family,
 						const struct command *cmd);
+void display_dictionary(struct l_dbus_message_iter *dict,
+			const struct display_dict_mapping *mapping,
+			const char *margin,
+			int name_column_width, int value_column_width);
 
 void display_refresh_timeout_set(void);
 void display_refresh_reset(void);
-- 
2.26.2

[PATCH v2 4/5] client: implement "ap <wlan> show"

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3790 bytes --]

This command uses GetDiagnostics to show a list of connected
clients and some information about them. The information
contained for each connected station nearly maps 1:1 with the
station diagnostics information shown in "station <wlan> show"
apart from "ConnectedBss" which is now "Address".
---
 client/ap.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/client/ap.c b/client/ap.c
index a6a2681b..6e2e96c4 100644
--- a/client/ap.c
+++ b/client/ap.c
@@ -189,6 +189,76 @@ static enum cmd_status cmd_stop(const char *device_name, char **argv, int argc)
 	return CMD_STATUS_TRIGGERED;
 }
 
+static struct proxy_interface_type ap_diagnostic_interface_type = {
+	.interface = IWD_AP_DIAGNOSTIC_INTERFACE,
+};
+
+static void display_bitrate_100kbps(struct l_dbus_message_iter *variant,
+				const char *key, const char *margin,
+				int name_column_width, int value_column_width)
+{
+	uint32_t rate;
+
+	l_dbus_message_iter_get_variant(variant, "u", &rate);
+
+	display("%s%-*s%-*u Kbit/s\n", margin, name_column_width, key,
+			value_column_width, rate * 100);
+}
+
+static const struct display_dict_mapping diagnostic_mapping[] = {
+	{ "Address", 's', NULL },
+	{ "RxMode", 's', NULL },
+	{ "TxMode", 's', NULL },
+	{ "RxBitrate", 0, display_bitrate_100kbps },
+	{ "TxBitrate", 0, display_bitrate_100kbps },
+	{ "ExpectedThroughput", 'u', "Kbit/s" },
+	{ "RSSI", 'n', "dBm" },
+	{ "RxMCS", 'y', NULL },
+	{ "TxMCS", 'y', NULL},
+	{ NULL }
+};
+
+static void ap_get_diagnostics_callback(struct l_dbus_message *message,
+					void *user_data)
+{
+	struct l_dbus_message_iter array;
+	struct l_dbus_message_iter iter;
+	uint16_t idx = 0;
+	char client_num[15];
+
+	if (dbus_message_has_error(message))
+		return;
+
+	if (!l_dbus_message_get_arguments(message, "aa{sv}", &array)) {
+		display("Failed to parse GetDiagnostics message");
+		return;
+	}
+
+	while (l_dbus_message_iter_next_entry(&array, &iter)) {
+		sprintf(client_num, "Client %u", idx++);
+		display_table_header(client_num, "%-*s%-*s",
+					20, "Property", 20, "Value");
+		display_dictionary(&iter, diagnostic_mapping, "", 20, 20);
+		display_table_footer();
+	}
+}
+
+static enum cmd_status cmd_show(const char *device_name, char **argv, int argc)
+{
+	const struct proxy_interface *ap_diagnostic =
+		device_proxy_find(device_name, IWD_AP_DIAGNOSTIC_INTERFACE);
+
+	if (!ap_diagnostic) {
+		display("No ap on device: '%s'\n", device_name);
+		return CMD_STATUS_INVALID_VALUE;
+	}
+
+	proxy_interface_method_call(ap_diagnostic, "GetDiagnostics", "",
+					ap_get_diagnostics_callback);
+
+	return CMD_STATUS_TRIGGERED;
+}
+
 static const struct command ap_commands[] = {
 	{ NULL, "list", NULL, cmd_list, "List devices in AP mode", true },
 	{ "<wlan>", "start", "<\"network name\"> <passphrase>", cmd_start,
@@ -196,6 +266,7 @@ static const struct command ap_commands[] = {
 		"name\" with\n\t\t\t\t\t\t    a passphrase" },
 	{ "<wlan>", "stop", NULL,   cmd_stop, "Stop a started access\n"
 		"\t\t\t\t\t\t    point" },
+	{ "<wlan", "show", NULL, cmd_show, "Show AP info", false },
 	{ }
 };
 
@@ -236,6 +307,7 @@ COMMAND_FAMILY(ap_command_family, ap_command_family_init,
 static int ap_interface_init(void)
 {
 	proxy_interface_type_register(&ap_interface_type);
+	proxy_interface_type_register(&ap_diagnostic_interface_type);
 
 	return 0;
 }
@@ -243,6 +315,7 @@ static int ap_interface_init(void)
 static void ap_interface_exit(void)
 {
 	proxy_interface_type_unregister(&ap_interface_type);
+	proxy_interface_type_unregister(&ap_diagnostic_interface_type);
 }
 
 INTERFACE_TYPE(ap_interface_type, ap_interface_init, ap_interface_exit)
-- 
2.26.2

[PATCH v2 5/5] client: update station to use display_dictionary

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3158 bytes --]

---
 client/station.c | 75 +++++++++++++++++-------------------------------
 1 file changed, 27 insertions(+), 48 deletions(-)

diff --git a/client/station.c b/client/station.c
index 93b1a4da..1085e18d 100644
--- a/client/station.c
+++ b/client/station.c
@@ -593,12 +593,36 @@ static enum cmd_status cmd_scan(const char *device_name,
 	return CMD_STATUS_TRIGGERED;
 }
 
+static void display_bitrate_100kbps(struct l_dbus_message_iter *variant,
+				const char *key, const char *margin,
+				int name_column_width,
+				int value_column_width)
+{
+	uint32_t rate;
+
+	l_dbus_message_iter_get_variant(variant, "u", &rate);
+
+	display("%s%-*s%-*u Kbit/s\n", margin, name_column_width, key,
+			value_column_width, rate * 100);
+}
+
+static const struct display_dict_mapping diagnostic_mapping[] = {
+	{ "ConnectedBss", 's', NULL },
+	{ "RxMode", 's', NULL },
+	{ "TxMode", 's', NULL },
+	{ "RxBitrate", 0, display_bitrate_100kbps },
+	{ "TxBitrate", 0, display_bitrate_100kbps },
+	{ "ExpectedThroughput", 'u', "Kbit/s" },
+	{ "RSSI", 'n', "dBm" },
+	{ "RxMCS", 'y', NULL },
+	{ "TxMCS", 'y', NULL},
+	{ NULL }
+};
+
 static void get_diagnostics_callback(struct l_dbus_message *message,
 					void *user_data)
 {
 	struct l_dbus_message_iter iter;
-	struct l_dbus_message_iter variant;
-	const char *key;
 
 	if (dbus_message_has_error(message))
 		return;
@@ -608,52 +632,7 @@ static void get_diagnostics_callback(struct l_dbus_message *message,
 		goto done;
 	}
 
-	while (l_dbus_message_iter_next_entry(&iter, &key, &variant)) {
-		const char *s_value;
-		uint32_t u_value;
-		int16_t i_value;
-		uint8_t y_value;
-
-		if (!strcmp(key, "ConnectedBss") || !strcmp(key, "RxMode") ||
-				!strcmp(key, "TxMode")) {
-			/* String variants with no special handling */
-
-			l_dbus_message_iter_get_variant(&variant, "s",
-							&s_value);
-
-			display("%s%*s  %-*s%-*s\n", MARGIN, 8, "", 20,
-				key, 47, s_value);
-		} else if (!strcmp(key, "RxBitrate") ||
-				!strcmp(key, "TxBitrate")) {
-			/* Bitrates expressed in 100Kbit/s */
-
-			l_dbus_message_iter_get_variant(&variant, "u",
-							&u_value);
-			display("%s%*s  %-*s%u Kbit/s\n", MARGIN, 8, "", 20,
-				key, u_value * 100);
-		} else if (!strcmp(key, "ExpectedThroughput")) {
-			/* ExpectedThroughput expressed in Kbit/s */
-
-			l_dbus_message_iter_get_variant(&variant, "u",
-							&u_value);
-			display("%s%*s  %-*s%u Kbit/s\n", MARGIN, 8, "", 20,
-				key, u_value);
-		} else if (!strcmp(key, "RSSI")) {
-			/* RSSI expressed in dBm */
-
-			l_dbus_message_iter_get_variant(&variant, "n",
-							&i_value);
-			display("%s%*s  %-*s%i dBm\n", MARGIN, 8, "", 20,
-				key, i_value);
-		} else if (!strcmp(key, "RxMCS") || !strcmp(key, "TxMCS")) {
-			/* MCS index's are single byte integers */
-
-			l_dbus_message_iter_get_variant(&variant, "y",
-							&y_value);
-			display("%s%*s  %-*s%u\n", MARGIN, 8, "", 20,
-				key, y_value);
-		}
-	}
+	display_dictionary(&iter, diagnostic_mapping, "            ", 20, 20);
 
 done:
 	/* Finish the table started by cmd_show */
-- 
2.26.2

Re: [PATCH v2 3/5] client: implement display_dictionary

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

Hi James,

On 1/21/21 12:11 PM, James Prestwood wrote:
> This takes a Dbus iterator which has been entered into a
> dictionary and prints out each key and value. It requires
> a mapping which maps keys to types and units. For simple
> cases the mapping will consist of a dbus type character
> and a units string, e.g. dBm, Kbit/s etc. For more complex
> printing which requires processing the value the 'units'
> void* cant be set to a function which can be custom written
> to handle the value.
> ---
>   client/display.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++
>   client/display.h | 22 ++++++++++++
>   2 files changed, 116 insertions(+)
> 
> v2:
>   * Generalized the printing of DBus dictionaries.
> 

<snip>

This looks good to me.  Question:

> +/*
> + * Maps dictionary keys to types/units. 'type' should be a valid DBus type, or
> + * zero for displaying in a custom fashion. When the display needs to be
> + * customized 'units' should point to a custom display function of the form
> + * display_dict_custom_func_t which should display the entire value as well
> + * as any units required.
> + */
> +struct display_dict_mapping {
> +	const char *key;
> +	char type;
> +	void *units;

Can we use a union here to cut down on the casting?  Something like

union {
	const char *units;
	display_dict_custom_func_t custom;
};

> +};
>   
Regards,
-Denis

Re: [PATCH v2 3/5] client: implement display_dictionary

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Hi Denis,

> > +/*
> > + * Maps dictionary keys to types/units. 'type' should be a valid
> > DBus type, or
> > + * zero for displaying in a custom fashion. When the display needs
> > to be
> > + * customized 'units' should point to a custom display function of
> > the form
> > + * display_dict_custom_func_t which should display the entire
> > value as well
> > + * as any units required.
> > + */
> > +struct display_dict_mapping {
> > +	const char *key;
> > +	char type;
> > +	void *units;
> 
> Can we use a union here to cut down on the casting?  Something like
> 
> union {
> 	const char *units;
> 	display_dict_custom_func_t custom;
> };

Sure. This is much more intuative as well.

Thanks,
James
> 
> > +};
> >   
> Regards,
> -Denis

Re: [PATCH v2 4/5] client: implement "ap <wlan> show"

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]

Hi James,

On 1/21/21 12:11 PM, James Prestwood wrote:
> This command uses GetDiagnostics to show a list of connected
> clients and some information about them. The information
> contained for each connected station nearly maps 1:1 with the
> station diagnostics information shown in "station <wlan> show"
> apart from "ConnectedBss" which is now "Address".
> ---
>   client/ap.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/client/ap.c b/client/ap.c
> index a6a2681b..6e2e96c4 100644
> --- a/client/ap.c
> +++ b/client/ap.c
> @@ -189,6 +189,76 @@ static enum cmd_status cmd_stop(const char *device_name, char **argv, int argc)
>   	return CMD_STATUS_TRIGGERED;
>   }
>   
> +static struct proxy_interface_type ap_diagnostic_interface_type = {
> +	.interface = IWD_AP_DIAGNOSTIC_INTERFACE,
> +};
> +
> +static void display_bitrate_100kbps(struct l_dbus_message_iter *variant,
> +				const char *key, const char *margin,
> +				int name_column_width, int value_column_width)
> +{
> +	uint32_t rate;
> +
> +	l_dbus_message_iter_get_variant(variant, "u", &rate);
> +
> +	display("%s%-*s%-*u Kbit/s\n", margin, name_column_width, key,
> +			value_column_width, rate * 100);
> +}

You don't want this in display.[ch] somewhere?

> +
> +static const struct display_dict_mapping diagnostic_mapping[] = {
> +	{ "Address", 's', NULL },
> +	{ "RxMode", 's', NULL },
> +	{ "TxMode", 's', NULL },
> +	{ "RxBitrate", 0, display_bitrate_100kbps },
> +	{ "TxBitrate", 0, display_bitrate_100kbps },
> +	{ "ExpectedThroughput", 'u', "Kbit/s" },
> +	{ "RSSI", 'n', "dBm" },
> +	{ "RxMCS", 'y', NULL },
> +	{ "TxMCS", 'y', NULL},
> +	{ NULL }
> +};
> +
> +static void ap_get_diagnostics_callback(struct l_dbus_message *message,
> +					void *user_data)
> +{
> +	struct l_dbus_message_iter array;
> +	struct l_dbus_message_iter iter;
> +	uint16_t idx = 0;
> +	char client_num[15];
> +
> +	if (dbus_message_has_error(message))
> +		return;

Does this need to print something as well?

> +
> +	if (!l_dbus_message_get_arguments(message, "aa{sv}", &array)) {
> +		display("Failed to parse GetDiagnostics message");
> +		return;
> +	}
> +
> +	while (l_dbus_message_iter_next_entry(&array, &iter)) {
> +		sprintf(client_num, "Client %u", idx++);
> +		display_table_header(client_num, "%-*s%-*s",
> +					20, "Property", 20, "Value");
> +		display_dictionary(&iter, diagnostic_mapping, "", 20, 20);
> +		display_table_footer();
> +	}
> +}
> +
> +static enum cmd_status cmd_show(const char *device_name, char **argv, int argc)
> +{
> +	const struct proxy_interface *ap_diagnostic =
> +		device_proxy_find(device_name, IWD_AP_DIAGNOSTIC_INTERFACE);
> +
> +	if (!ap_diagnostic) {
> +		display("No ap on device: '%s'\n", device_name);
> +		return CMD_STATUS_INVALID_VALUE;
> +	}
> +
> +	proxy_interface_method_call(ap_diagnostic, "GetDiagnostics", "",
> +					ap_get_diagnostics_callback);
> +
> +	return CMD_STATUS_TRIGGERED;
> +}
> +
>   static const struct command ap_commands[] = {
>   	{ NULL, "list", NULL, cmd_list, "List devices in AP mode", true },
>   	{ "<wlan>", "start", "<\"network name\"> <passphrase>", cmd_start,

Regards,
-Denis

Re: [PATCH v2 4/5] client: implement "ap <wlan> show"

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

Hi,

> > +static void display_bitrate_100kbps(struct l_dbus_message_iter
> > *variant,
> > +				const char *key, const char *margin,
> > +				int name_column_width, int
> > value_column_width)
> > +{
> > +	uint32_t rate;
> > +
> > +	l_dbus_message_iter_get_variant(variant, "u", &rate);
> > +
> > +	display("%s%-*s%-*u Kbit/s\n", margin, name_column_width, key,
> > +			value_column_width, rate * 100);
> > +}
> 
> You don't want this in display.[ch] somewhere?

I could do that. Its the same for station/ap. The reason I put it here
is because its not really a generalized display API since its so unique
to this specific value. But I'm fine either way, I'll put it in
display.c/h so we don't have it duplicated.

> 
> > +
> > +static const struct display_dict_mapping diagnostic_mapping[] = {
> > +	{ "Address", 's', NULL },
> > +	{ "RxMode", 's', NULL },
> > +	{ "TxMode", 's', NULL },
> > +	{ "RxBitrate", 0, display_bitrate_100kbps },
> > +	{ "TxBitrate", 0, display_bitrate_100kbps },
> > +	{ "ExpectedThroughput", 'u', "Kbit/s" },
> > +	{ "RSSI", 'n', "dBm" },
> > +	{ "RxMCS", 'y', NULL },
> > +	{ "TxMCS", 'y', NULL},
> > +	{ NULL }
> > +};
> > +
> > +static void ap_get_diagnostics_callback(struct l_dbus_message
> > *message,
> > +					void *user_data)
> > +{
> > +	struct l_dbus_message_iter array;
> > +	struct l_dbus_message_iter iter;
> > +	uint16_t idx = 0;
> > +	char client_num[15];
> > +
> > +	if (dbus_message_has_error(message))
> > +		return;
> 
> Does this need to print something as well?

That function does more than just check (though you wouldn't guess it
from the name). It ends up converting the error into a string and
printing it.

Thanks,
James

Re: [PATCH v2 1/5] station: commonize the building of diagnostic dict

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

Hi James,

On 1/21/21 12:11 PM, James Prestwood wrote:
> AP mode will use the same structure for its diagnostic interface
> and mostly the same dictionary keys. Apart from ConnectedBss and
> Address being different, the remainder are the same so the
> netdev_station_info to DBus dictionary conversion has been made
> common so both station and AP can use it to build its diagnostic
> dictionaries.
> ---
>   src/station.c | 71 ++++++++++++++++++++++++++++++---------------------
>   src/station.h |  3 +++
>   2 files changed, 45 insertions(+), 29 deletions(-)
> 
> v2:
>   * Generalized the dictionary building so both station and AP could
>     use it.
> 

<snip>

> +
> +bool station_info_to_dict(const struct netdev_station_info *info,
> +				struct l_dbus_message_builder *builder);
> 

This doesn't feel right to have ap (and possibly p2p, adhoc, etc) depend on 
station for this.  Can we move the netdev_station_info structure definition to 
src/common.h or maybe diagnostic.h (diagnostic_station_info?) or so and add this 
to diagnostic.[ch] instead?

Regards,
-Denis

Re: [PATCH v2 1/5] station: commonize the building of diagnostic dict

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

On Thu, 2021-01-21 at 14:08 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 1/21/21 12:11 PM, James Prestwood wrote:
> > AP mode will use the same structure for its diagnostic interface
> > and mostly the same dictionary keys. Apart from ConnectedBss and
> > Address being different, the remainder are the same so the
> > netdev_station_info to DBus dictionary conversion has been made
> > common so both station and AP can use it to build its diagnostic
> > dictionaries.
> > ---
> >   src/station.c | 71 ++++++++++++++++++++++++++++++--------------
> > -------
> >   src/station.h |  3 +++
> >   2 files changed, 45 insertions(+), 29 deletions(-)
> > 
> > v2:
> >   * Generalized the dictionary building so both station and AP
> > could
> >     use it.
> > 
> 
> <snip>
> 
> > +
> > +bool station_info_to_dict(const struct netdev_station_info *info,
> > +				struct l_dbus_message_builder
> > *builder);
> > 
> 
> This doesn't feel right to have ap (and possibly p2p, adhoc, etc)
> depend on 
> station for this.  Can we move the netdev_station_info structure
> definition to 
> src/common.h or maybe diagnostic.h (diagnostic_station_info?) or so
> and add this 
> to diagnostic.[ch] instead?

Sure, doing it in diagnostic.[ch] seems like the right way to go.

> 
> Regards,
> -Denis

Re: [PATCH v2 4/5] client: implement "ap <wlan> show"

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

Hi James,

On 1/21/21 2:05 PM, James Prestwood wrote:
> Hi,
> 
>>> +static void display_bitrate_100kbps(struct l_dbus_message_iter
>>> *variant,
>>> +				const char *key, const char *margin,
>>> +				int name_column_width, int
>>> value_column_width)
>>> +{
>>> +	uint32_t rate;
>>> +
>>> +	l_dbus_message_iter_get_variant(variant, "u", &rate);
>>> +
>>> +	display("%s%-*s%-*u Kbit/s\n", margin, name_column_width, key,
>>> +			value_column_width, rate * 100);
>>> +}
>>
>> You don't want this in display.[ch] somewhere?
> 
> I could do that. Its the same for station/ap. The reason I put it here
> is because its not really a generalized display API since its so unique
> to this specific value. But I'm fine either way, I'll put it in
> display.c/h so we don't have it duplicated.

Could also put this into diagnostic.[ch] or something as well.  In theory even 
the dict_mapping parts can be shared since they're mostly the same, and 
Address/CurrentBss won't occur in the same message.

> 
>>
>>> +
>>> +static const struct display_dict_mapping diagnostic_mapping[] = {
>>> +	{ "Address", 's', NULL },
>>> +	{ "RxMode", 's', NULL },
>>> +	{ "TxMode", 's', NULL },
>>> +	{ "RxBitrate", 0, display_bitrate_100kbps },
>>> +	{ "TxBitrate", 0, display_bitrate_100kbps },
>>> +	{ "ExpectedThroughput", 'u', "Kbit/s" },
>>> +	{ "RSSI", 'n', "dBm" },
>>> +	{ "RxMCS", 'y', NULL },
>>> +	{ "TxMCS", 'y', NULL},
>>> +	{ NULL }
>>> +};
>>> +
>>> +static void ap_get_diagnostics_callback(struct l_dbus_message
>>> *message,
>>> +					void *user_data)
>>> +{
>>> +	struct l_dbus_message_iter array;
>>> +	struct l_dbus_message_iter iter;
>>> +	uint16_t idx = 0;
>>> +	char client_num[15];
>>> +
>>> +	if (dbus_message_has_error(message))
>>> +		return;
>>
>> Does this need to print something as well?
> 
> That function does more than just check (though you wouldn't guess it
> from the name). It ends up converting the error into a string and
> printing it.

Ok, cool.

Regards,
-Denis

Re: [PATCH v2 4/5] client: implement "ap <wlan> show"

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

On Thu, 2021-01-21 at 14:12 -0600, Denis Kenzior wrote:
> Hi James,
> 
> On 1/21/21 2:05 PM, James Prestwood wrote:
> > Hi,
> > 
> > > > +static void display_bitrate_100kbps(struct l_dbus_message_iter
> > > > *variant,
> > > > +				const char *key, const char
> > > > *margin,
> > > > +				int name_column_width, int
> > > > value_column_width)
> > > > +{
> > > > +	uint32_t rate;
> > > > +
> > > > +	l_dbus_message_iter_get_variant(variant, "u", &rate);
> > > > +
> > > > +	display("%s%-*s%-*u Kbit/s\n", margin,
> > > > name_column_width, key,
> > > > +			value_column_width, rate * 100);
> > > > +}
> > > 
> > > You don't want this in display.[ch] somewhere?
> > 
> > I could do that. Its the same for station/ap. The reason I put it
> > here
> > is because its not really a generalized display API since its so
> > unique
> > to this specific value. But I'm fine either way, I'll put it in
> > display.c/h so we don't have it duplicated.
> 
> Could also put this into diagnostic.[ch] or something as well.  In
> theory even 
> the dict_mapping parts can be shared since they're mostly the same,
> and 
> Address/CurrentBss won't occur in the same message.

Yeah true, Address and CurrentBss could both be in the same mapping
without any problems. I guess if we enforce the AP and station
dictionaries to never have duplicate names which have different
meanings they could all be defined in a single mapping.

> 
> > > > +
> > > > +static const struct display_dict_mapping diagnostic_mapping[]
> > > > = {
> > > > +	{ "Address", 's', NULL },
> > > > +	{ "RxMode", 's', NULL },
> > > > +	{ "TxMode", 's', NULL },
> > > > +	{ "RxBitrate", 0, display_bitrate_100kbps },
> > > > +	{ "TxBitrate", 0, display_bitrate_100kbps },
> > > > +	{ "ExpectedThroughput", 'u', "Kbit/s" },
> > > > +	{ "RSSI", 'n', "dBm" },
> > > > +	{ "RxMCS", 'y', NULL },
> > > > +	{ "TxMCS", 'y', NULL},
> > > > +	{ NULL }
> > > > +};
> > > > +
> > > > +static void ap_get_diagnostics_callback(struct l_dbus_message
> > > > *message,
> > > > +					void *user_data)
> > > > +{
> > > > +	struct l_dbus_message_iter array;
> > > > +	struct l_dbus_message_iter iter;
> > > > +	uint16_t idx = 0;
> > > > +	char client_num[15];
> > > > +
> > > > +	if (dbus_message_has_error(message))
> > > > +		return;
> > > 
> > > Does this need to print something as well?
> > 
> > That function does more than just check (though you wouldn't guess
> > it
> > from the name). It ends up converting the error into a string and
> > printing it.
> 
> Ok, cool.
> 
> Regards,
> -Denis