[PATCH 2/3] iwd: Connection of hidden networks

[PATCH 2/3] iwd: Connection of hidden networks

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Implementation of hidden networks connection.
---
 plugins/iwd.c | 227 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 208 insertions(+), 19 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index 00578f4d4ab3..961d53196207 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -121,6 +121,17 @@ struct iwd_hidden_ap {
 	int16_t signal_strength;
 };
 
+struct hidden_params {
+	char* ssid;
+	unsigned int ssid_len;
+	char *identity;
+	char *passphrase;
+	char *security;
+	gpointer user_data;
+	char *station_path;
+	char *network_path;
+};
+
 struct iwd_station {
 	GDBusProxy *proxy;
 	char *path;
@@ -129,6 +140,8 @@ struct iwd_station {
 	bool scanning;
 
 	GHashTable *hidden_aps;
+	struct hidden_params *hidden;
+	bool postpone_hidden;
 };
 
 struct iwd_ap {
@@ -560,6 +573,79 @@ static int cm_device_disable(struct connman_device *device)
 	return set_device_powered(device, false);
 }
 
+static void hidden_network_connect_append(DBusMessageIter *iter,
+		void *user_data)
+{
+	struct hidden_params *hidden = user_data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &hidden->ssid);
+}
+
+static void update_signal_strength(struct iwd_station *iwds);
+static void cm_hidden_network_connect_cb(DBusMessage *message, void *user_data)
+{
+	struct hidden_params *hidden = user_data;
+	struct iwd_network *iwdn = NULL;
+	struct iwd_station *iwds;
+	const char *dbus_error;
+
+	if (!hidden)
+		return;
+
+	iwds = g_hash_table_lookup(stations, hidden->station_path);
+	if (!iwds)
+		return;
+
+	iwds->hidden = NULL;
+
+	if (hidden->network_path)
+		iwdn = g_hash_table_lookup(networks, hidden->network_path);
+
+	if (dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_ERROR) {
+		if (iwdn) {
+			update_network_connected(iwdn);
+			update_signal_strength(iwds);
+		}
+		return;
+	}
+
+	dbus_error = dbus_message_get_error_name(message);
+	if (!strcmp(dbus_error, "net.connman.iwd.InProgress") ||
+		!strcmp(dbus_error, "net.connman.iwd.AlreadyProvisioned"))
+		return;
+
+	DBG("Hidden connection failed for station %s: %s",
+		hidden->station_path,
+		dbus_error);
+
+	if (!iwdn) {
+		connman_error("Hidden network not found for station %s",
+			hidden->station_path);
+		return;
+	}
+
+	if (!strcmp(dbus_error, "net.connman.iwd.Failed"))
+		connman_network_set_error(iwdn->network,
+				CONNMAN_NETWORK_ERROR_INVALID_KEY);
+	else if (!iwdn->autoconnect)
+		connman_network_set_error(iwdn->network,
+				CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+}
+
+static void hidden_params_free(gpointer data)
+{
+	struct hidden_params *hidden = data;
+
+	g_free(hidden->ssid);
+	g_free(hidden->identity);
+	g_free(hidden->passphrase);
+	g_free(hidden->security);
+	connman_network_clear_hidden(hidden->user_data);
+	g_free(hidden->station_path);
+	g_free(hidden->network_path);
+	g_free(hidden);
+}
+
 static void cm_device_scan_cb(DBusMessage *message, void *user_data)
 {
 	const char *path = user_data;
@@ -574,6 +660,23 @@ static void cm_device_scan_cb(DBusMessage *message, void *user_data)
 
 		DBG("%s scan failed: %s", path, dbus_error);
 	}
+
+	if (!iwds->hidden)
+		return;
+
+	if (!iwds->postpone_hidden) {
+		hidden_params_free(iwds->hidden);
+		iwds->hidden = NULL;
+	} else {
+		iwds->postpone_hidden = false;
+		if (!g_dbus_proxy_method_call(iwds->proxy,
+				"ConnectHiddenNetwork",
+				hidden_network_connect_append,
+				cm_hidden_network_connect_cb,
+				iwds->hidden, hidden_params_free)) {
+			iwds->hidden = NULL;
+		}
+	}
 }
 
 static int cm_device_scan(struct connman_device *device,
@@ -581,19 +684,72 @@ static int cm_device_scan(struct connman_device *device,
 {
 	struct iwd_device *iwdd = connman_device_get_data(device);
 	struct iwd_station *iwds;
+	bool scanning;
+
+	if (!iwdd)
+		return -ENODEV;
 
 	if (strcmp(iwdd->mode, "station"))
 		return -EINVAL;
 
+	scanning = connman_device_get_scanning(device,
+		CONNMAN_SERVICE_TYPE_WIFI);
+
 	iwds = g_hash_table_lookup(stations, iwdd->path);
 	if (!iwds)
 		return -EIO;
 
-	if (!g_dbus_proxy_method_call(iwds->proxy, "Scan",
+	if (!params->ssid || params->ssid_len == 0 || params->ssid_len > 32) {
+		if (scanning)
+			return -EALREADY;
+
+		if (!g_dbus_proxy_method_call(iwds->proxy, "Scan",
 			NULL, cm_device_scan_cb, g_strdup(iwds->path), g_free))
-		return -EIO;
+			return -EIO;
 
-	return -EINPROGRESS;
+		return -EINPROGRESS;
+	} else {
+		struct hidden_params *hidden;
+
+		if (scanning && iwds->hidden && iwds->postpone_hidden)
+			return -EALREADY;
+
+		hidden = g_try_new0(struct hidden_params, 1);
+		if (!hidden)
+			return -ENOMEM;
+
+		if (iwds->hidden) {
+			hidden_params_free(iwds->hidden);
+			iwds->hidden = NULL;
+		}
+
+		hidden->ssid = g_strndup (params->ssid, params->ssid_len);
+		hidden->ssid_len = params->ssid_len;
+		hidden->identity = g_strdup(params->identity);
+		hidden->passphrase = g_strdup(params->passphrase);
+		hidden->security = g_strdup(params->security);
+		hidden->user_data = params->user_data;
+		hidden->station_path = g_strdup(iwds->path);
+		iwds->hidden = hidden;
+
+		if (scanning) {
+			/* Let's keep this active scan for later,
+			 * when current scan will be over. */
+			iwds->postpone_hidden = true;
+
+			return 0;
+		}
+
+		if (!g_dbus_proxy_method_call(iwds->proxy,
+				"ConnectHiddenNetwork",
+				hidden_network_connect_append,
+				cm_hidden_network_connect_cb,
+				hidden, hidden_params_free)) {
+			iwds->hidden = NULL;
+			return -EIO;
+		}
+		return 0;
+	}
 }
 
 static struct connman_device_driver device_driver = {
@@ -917,7 +1073,10 @@ static char *create_identifier(const char *path, const char *security)
 
 static void add_network(const char *path, struct iwd_network *iwdn)
 {
+	unsigned int ssid_len = 0;
+	struct iwd_station *iwds;
 	struct iwd_device *iwdd;
+	const char *security;
 	char *identifier;
 	int index;
 
@@ -926,33 +1085,56 @@ static void add_network(const char *path, struct iwd_network *iwdn)
 		return;
 
 	identifier = create_identifier(path, iwdn->type);
-	iwdn->network = connman_network_create(identifier,
+	iwdn->network = connman_device_get_network(iwdd->device, identifier);
+	if (!iwdn->network) {
+		iwdn->network = connman_network_create(identifier,
 					CONNMAN_NETWORK_TYPE_WIFI);
 
-	index = connman_inet_ifindex(iwdd->name);
-	if (index >= 0)
-		connman_network_set_index(iwdn->network, index);
+		index = connman_inet_ifindex(iwdd->name);
+		if (index >= 0)
+			connman_network_set_index(iwdn->network, index);
 
-	connman_network_set_data(iwdn->network, iwdn);
+		connman_network_set_data(iwdn->network, iwdn);
+
+		if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
+			connman_network_unref(iwdn->network);
+			iwdn->network = NULL;
+			return;
+		}
+	}
 
 	connman_network_set_name(iwdn->network, iwdn->name);
-	if (iwdn->name)
+	if (iwdn->name) {
+		ssid_len = strlen(iwdn->name);
 		connman_network_set_blob(iwdn->network, "WiFi.SSID", iwdn->name,
-					strlen(iwdn->name));
+					ssid_len);
+	}
+
+	security = security_remap(iwdn->type);
 	connman_network_set_string(iwdn->network, "WiFi.Security",
-					security_remap(iwdn->type));
+					security);
 	connman_network_set_string(iwdn->network, "WiFi.Mode", "managed");
-
-	if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
-		connman_network_unref(iwdn->network);
-		iwdn->network = NULL;
-		return;
-	}
 	iwdn->iwdd = iwdd;
 
 	connman_network_set_available(iwdn->network, true);
 	connman_network_set_group(iwdn->network, identifier);
 
+	iwds = g_hash_table_lookup(stations, iwdn->iwdd->path);
+	if (iwds && iwds->hidden &&
+			!g_strcmp0(iwds->hidden->security, security) &&
+			iwds->hidden->ssid && iwdn->name &&
+			iwds->hidden->ssid_len == ssid_len &&
+			!memcmp(iwds->hidden->ssid, iwdn->name, ssid_len)) {
+		connman_network_set_associating(iwdn->network, true);
+
+		iwds->hidden->network_path = g_strdup(iwdn->path);
+		connman_network_connect_hidden(iwdn->network,
+				iwds->hidden->identity,
+				iwds->hidden->passphrase,
+				iwds->hidden->user_data);
+		iwds->hidden->user_data = NULL;
+	}
+
 	g_free(identifier);
 }
 
@@ -1351,8 +1533,10 @@ static void station_property_change(GDBusProxy *proxy, const char *name,
 				connman_device_set_scanning(iwdd->device,
 					CONNMAN_SERVICE_TYPE_WIFI, true);
 		} else {
 			update_signal_strength(iwds);
-			update_hidden_networks(iwds);
+
+			if (!iwds->hidden)
+				update_hidden_networks(iwds);
 		}
 
 
@@ -1474,6 +1659,7 @@ static void station_free(gpointer data)
 	g_free(iwds->path);
 	g_free(iwds->connected_network);
 	g_hash_table_destroy(iwds->hidden_aps);
+	g_free(iwds->hidden);
 	g_free(iwds);
 }
 
@@ -1856,6 +2042,9 @@ static void create_station(GDBusProxy *proxy)
 	iwds->hidden_aps = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
 			hidden_ap_free);
 
+	iwds->hidden = NULL;
+	iwds->postpone_hidden = false;
+
 	DBG("state '%s' connected_network %s scanning %d",
 		iwds->state, iwds->connected_network, iwds->scanning);
 
-- 
2.25.1

Re: [PATCH 2/3] iwd: Connection of hidden networks

[Daniel Wagner]

Hi Emmanuel,

On Fri, Oct 01, 2021 at 03:27:06PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Implementation of hidden networks connection.
> +static void update_signal_strength(struct iwd_station *iwds);
> +static void cm_hidden_network_connect_cb(DBusMessage *message, void *user_data)
> +{
> +	struct hidden_params *hidden = user_data;
> +	struct iwd_network *iwdn = NULL;
> +	struct iwd_station *iwds;
> +	const char *dbus_error;
> +
> +	if (!hidden)
> +		return;
> +
> +	iwds = g_hash_table_lookup(stations, hidden->station_path);
> +	if (!iwds)
> +		return;
> +
> +	iwds->hidden = NULL;

I assume iwds->hidden is supposed to NULL, right? If it's not there is
bug? If this is the case it might be better to add a check and
warn/error.

I skimmed through the rest of the patch. Most looks straight forward,
but I have to say the hidden network feature always screws up the code
base. Thought this is not your fault, it's the stupid spec.

Thanks,
Daniel

RE: [PATCH 2/3] iwd: Connection of hidden networks

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Implementation of hidden networks connection.
---
 plugins/iwd.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 206 insertions(+), 18 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index 0f2b638caceb..4d009c3515a8 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -121,6 +121,17 @@ struct iwd_hidden_ap {
 	int16_t signal_strength;
 };
 
+struct hidden_params {
+	char* ssid;
+	unsigned int ssid_len;
+	char *identity;
+	char *passphrase;
+	char *security;
+	gpointer user_data;
+	char *station_path;
+	char *network_path;
+};
+
 struct iwd_station {
 	GDBusProxy *proxy;
 	char *path;
@@ -129,6 +140,8 @@ struct iwd_station {
 	bool scanning;
 
 	GHashTable *hidden_aps;
+	struct hidden_params *hidden;
+	bool postpone_hidden;
 };
 
 struct iwd_ap {
@@ -560,6 +573,79 @@ static int cm_device_disable(struct connman_device *device)
 	return set_device_powered(device, false);
 }
 
+static void hidden_network_connect_append(DBusMessageIter *iter,
+		void *user_data)
+{
+	struct hidden_params *hidden = user_data;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_STRING, &hidden->ssid);
+}
+
+static void update_signal_strength(struct iwd_station *iwds);
+static void cm_hidden_network_connect_cb(DBusMessage *message, void *user_data)
+{
+	struct hidden_params *hidden = user_data;
+	struct iwd_network *iwdn = NULL;
+	struct iwd_station *iwds;
+	const char *dbus_error;
+
+	if (!hidden)
+		return;
+
+	iwds = g_hash_table_lookup(stations, hidden->station_path);
+	if (!iwds)
+		return;
+
+	iwds->hidden = NULL;
+
+	if (hidden->network_path)
+		iwdn = g_hash_table_lookup(networks, hidden->network_path);
+
+	if (dbus_message_get_type(message) != DBUS_MESSAGE_TYPE_ERROR) {
+		if (iwdn) {
+			update_network_connected(iwdn);
+			update_signal_strength(iwds);
+		}
+		return;
+	}
+
+	dbus_error = dbus_message_get_error_name(message);
+	if (!strcmp(dbus_error, "net.connman.iwd.InProgress") ||
+		!strcmp(dbus_error, "net.connman.iwd.AlreadyProvisioned"))
+		return;
+
+	DBG("Hidden connection failed for station %s: %s",
+		hidden->station_path,
+		dbus_error);
+
+	if (!iwdn) {
+		connman_error("Hidden network not found for station %s",
+			hidden->station_path);
+		return;
+	}
+
+	if (!strcmp(dbus_error, "net.connman.iwd.Failed"))
+		connman_network_set_error(iwdn->network,
+				CONNMAN_NETWORK_ERROR_INVALID_KEY);
+	else if (!iwdn->autoconnect)
+		connman_network_set_error(iwdn->network,
+				CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+}
+
+static void hidden_params_free(gpointer data)
+{
+	struct hidden_params *hidden = data;
+
+	g_free(hidden->ssid);
+	g_free(hidden->identity);
+	g_free(hidden->passphrase);
+	g_free(hidden->security);
+	connman_network_clear_hidden(hidden->user_data);
+	g_free(hidden->station_path);
+	g_free(hidden->network_path);
+	g_free(hidden);
+}
+
 static void cm_device_scan_cb(DBusMessage *message, void *user_data)
 {
 	const char *path = user_data;
@@ -574,6 +660,23 @@ static void cm_device_scan_cb(DBusMessage *message, void *user_data)
 
 		DBG("%s scan failed: %s", path, dbus_error);
 	}
+
+	if (!iwds->hidden)
+		return;
+
+	if (!iwds->postpone_hidden) {
+		hidden_params_free(iwds->hidden);
+		iwds->hidden = NULL;
+	} else {
+		iwds->postpone_hidden = false;
+		if (!g_dbus_proxy_method_call(iwds->proxy,
+				"ConnectHiddenNetwork",
+				hidden_network_connect_append,
+				cm_hidden_network_connect_cb,
+				iwds->hidden, hidden_params_free)) {
+			iwds->hidden = NULL;
+		}
+	}
 }
 
 static int cm_device_scan(struct connman_device *device,
@@ -581,19 +684,72 @@ static int cm_device_scan(struct connman_device *device,
 {
 	struct iwd_device *iwdd = connman_device_get_data(device);
 	struct iwd_station *iwds;
+	bool scanning;
+
+	if (!iwdd)
+		return -ENODEV;
 
 	if (strcmp(iwdd->mode, "station"))
 		return -EINVAL;
 
+	scanning = connman_device_get_scanning(device,
+		CONNMAN_SERVICE_TYPE_WIFI);
+
 	iwds = g_hash_table_lookup(stations, iwdd->path);
 	if (!iwds)
 		return -EIO;
 
-	if (!g_dbus_proxy_method_call(iwds->proxy, "Scan",
+	if (!params->ssid || params->ssid_len == 0 || params->ssid_len > 32) {
+		if (scanning)
+			return -EALREADY;
+
+		if (!g_dbus_proxy_method_call(iwds->proxy, "Scan",
 			NULL, cm_device_scan_cb, g_strdup(iwds->path), g_free))
-		return -EIO;
+			return -EIO;
 
-	return -EINPROGRESS;
+		return -EINPROGRESS;
+	} else {
+		struct hidden_params *hidden;
+
+		if (scanning && iwds->hidden && iwds->postpone_hidden)
+			return -EALREADY;
+
+		hidden = g_new0(struct hidden_params, 1);
+		if (!hidden)
+			return -ENOMEM;
+
+		if (iwds->hidden) {
+			hidden_params_free(iwds->hidden);
+			iwds->hidden = NULL;
+		}
+
+		hidden->ssid = g_strndup (params->ssid, params->ssid_len);
+		hidden->ssid_len = params->ssid_len;
+		hidden->identity = g_strdup(params->identity);
+		hidden->passphrase = g_strdup(params->passphrase);
+		hidden->security = g_strdup(params->security);
+		hidden->user_data = params->user_data;
+		hidden->station_path = g_strdup(iwds->path);
+		iwds->hidden = hidden;
+
+		if (scanning) {
+			/* Let's keep this active scan for later,
+			 * when current scan will be over. */
+			iwds->postpone_hidden = true;
+
+			return 0;
+		}
+
+		if (!g_dbus_proxy_method_call(iwds->proxy,
+				"ConnectHiddenNetwork",
+				hidden_network_connect_append,
+				cm_hidden_network_connect_cb,
+				hidden, hidden_params_free)) {
+			iwds->hidden = NULL;
+			return -EIO;
+		}
+		return 0;
+	}
 }
 
 static struct connman_device_driver device_driver = {
@@ -917,7 +1073,10 @@ static char *create_identifier(const char *path, const char *security)
 
 static void add_network(const char *path, struct iwd_network *iwdn)
 {
+	unsigned int ssid_len = 0;
+	struct iwd_station *iwds;
 	struct iwd_device *iwdd;
+	const char *security;
 	char *identifier;
 	int index;
 
@@ -926,33 +1085,56 @@ static void add_network(const char *path, struct iwd_network *iwdn)
 		return;
 
 	identifier = create_identifier(path, iwdn->type);
-	iwdn->network = connman_network_create(identifier,
+	iwdn->network = connman_device_get_network(iwdd->device, identifier);
+	if (!iwdn->network) {
+		iwdn->network = connman_network_create(identifier,
 					CONNMAN_NETWORK_TYPE_WIFI);
 
-	index = connman_inet_ifindex(iwdd->name);
-	if (index >= 0)
-		connman_network_set_index(iwdn->network, index);
+		index = connman_inet_ifindex(iwdd->name);
+		if (index >= 0)
+			connman_network_set_index(iwdn->network, index);
 
-	connman_network_set_data(iwdn->network, iwdn);
+		connman_network_set_data(iwdn->network, iwdn);
+
+		if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
+			connman_network_unref(iwdn->network);
+			iwdn->network = NULL;
+			return;
+		}
+	}
 
 	connman_network_set_name(iwdn->network, iwdn->name);
-	if (iwdn->name)
+	if (iwdn->name) {
+		ssid_len = strlen(iwdn->name);
 		connman_network_set_blob(iwdn->network, "WiFi.SSID", iwdn->name,
-					strlen(iwdn->name));
+					ssid_len);
+	}
+
+	security = security_remap(iwdn->type);
 	connman_network_set_string(iwdn->network, "WiFi.Security",
-					security_remap(iwdn->type));
+					security);
 	connman_network_set_string(iwdn->network, "WiFi.Mode", "managed");
-
-	if (connman_device_add_network(iwdd->device, iwdn->network) < 0) {
-		connman_network_unref(iwdn->network);
-		iwdn->network = NULL;
-		return;
-	}
 	iwdn->iwdd = iwdd;
 
 	connman_network_set_available(iwdn->network, true);
 	connman_network_set_group(iwdn->network, identifier);
 
+	iwds = g_hash_table_lookup(stations, iwdn->iwdd->path);
+	if (iwds && iwds->hidden &&
+			!g_strcmp0(iwds->hidden->security, security) &&
+			iwds->hidden->ssid && iwdn->name &&
+			iwds->hidden->ssid_len == ssid_len &&
+			!memcmp(iwds->hidden->ssid, iwdn->name, ssid_len)) {
+		connman_network_set_associating(iwdn->network, true);
+
+		iwds->hidden->network_path = g_strdup(iwdn->path);
+		connman_network_connect_hidden(iwdn->network,
+				iwds->hidden->identity,
+				iwds->hidden->passphrase,
+				iwds->hidden->user_data);
+		iwds->hidden->user_data = NULL;
+	}
+
 	g_free(identifier);
 }
 
@@ -1352,7 +1534,9 @@ static void station_property_change(GDBusProxy *proxy, const char *name,
 					CONNMAN_SERVICE_TYPE_WIFI, true);
 		} else {
 			update_signal_strength(iwds);
-			update_hidden_networks(iwds);
+
+			if (!iwds->hidden)
+				update_hidden_networks(iwds);
 		}
 
 
@@ -1474,6 +1658,7 @@ static void station_free(gpointer data)
 	g_free(iwds->path);
 	g_free(iwds->connected_network);
 	g_hash_table_destroy(iwds->hidden_aps);
+	g_free(iwds->hidden);
 	g_free(iwds);
 }
 
@@ -1856,6 +2041,9 @@ static void create_station(GDBusProxy *proxy)
 	iwds->hidden_aps = g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
 			hidden_ap_free);
 
+	iwds->hidden = NULL;
+	iwds->postpone_hidden = false;
+
 	DBG("state '%s' connected_network %s scanning %d",
 		iwds->state, iwds->connected_network, iwds->scanning);
 
-- 
2.25.1
---------------------------------------------------------------------------------------------------------
Hi Daniel,

I have updated the patch with g_new0.

> +     iwds->hidden = NULL;
>
> I assume iwds->hidden is supposed to NULL, right? If it's not there is
> bug? If this is the case it might be better to add a check and
> warn/error.
It is NOT supposed to be NULL here. Where do you think it has been reset
before?
It only can be reset in case of errors in connect_hidden(), but the
callback shall not be called in this case. In the other case, if the network
is reached (good SSID, passphrase, ...), add_network() is called and hidden
is processed. In the other hand, hidden is ignored. In all cases, to inform
the end of the request, iwds->hidden shall be reset by cm_hidden_network_connect_cb.

> I skimmed through the rest of the patch. Most looks straight forward,
> but I have to say the hidden network feature always screws up the code
> base. Thought this is not your fault, it's the stupid spec.
Indeed, it is a tricky subject. At least, I have succeeded to pass my network
test campaign based on wpa-supplicant with this implementation and iwd 1.18.

Best Regards,

Emmanuel

Re: [PATCH 2/3] iwd: Connection of hidden networks

[Daniel Wagner]

Hi Emmanuel

On Mon, Oct 04, 2021 at 09:10:47AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Implementation of hidden networks connection.

Please send them as new patch. Thanks.

> > +     iwds->hidden = NULL;
> >
> > I assume iwds->hidden is supposed to NULL, right? If it's not there is
> > bug? If this is the case it might be better to add a check and
> > warn/error.
> It is NOT supposed to be NULL here. Where do you think it has been reset
> before?

In this case, why do you not release the resources associated with the
pointer? Isn't this a leak?

> It only can be reset in case of errors in connect_hidden(), but the
> callback shall not be called in this case. In the other case, if the network
> is reached (good SSID, passphrase, ...), add_network() is called and hidden
> is processed. In the other hand, hidden is ignored. In all cases, to inform
> the end of the request, iwds->hidden shall be reset by cm_hidden_network_connect_cb.
> 
> > I skimmed through the rest of the patch. Most looks straight forward,
> > but I have to say the hidden network feature always screws up the code
> > base. Thought this is not your fault, it's the stupid spec.
> Indeed, it is a tricky subject. At least, I have succeeded to pass my network
> test campaign based on wpa-supplicant with this implementation and iwd 1.18.

Ah this is good to know!

Daniel

RE: [PATCH 2/3] iwd: Connection of hidden networks

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Daniel,

> Please send them as new patch. Thanks.
Do you mean I should version the updated patches I have just sent back?

> In this case, why do you not release the resources associated with the
> pointer? Isn't this a leak?
I don't think so. It is done via the hidden_params_free(), given as argument of
g_dbus_proxy_method_call().
But I shall replace the g_free in station_free() by hidden_params_free.

Emmanuel

Re: [PATCH 2/3] iwd: Connection of hidden networks

[Daniel Wagner]

On Wed, Oct 06, 2021 at 12:44:05PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > Please send them as new patch. Thanks.
> Do you mean I should version the updated patches I have just sent back?

I would really appreciate if you could send your patches following the
normal Linux kernel patch submission work flow. That is send the patch
directly with 'git send-email'. git send-email gets the threading etc
all right. Please check the kernel docs on how to setup your git
environment. Doing so I can pick them directly from the mailing list and
don't have to manually copy&past stuff around.

> > In this case, why do you not release the resources associated with the
> > pointer? Isn't this a leak?
> I don't think so. It is done via the hidden_params_free(), given as argument of
> g_dbus_proxy_method_call().

In this case it would be better to set the pointer to NULL right after
the resource is freed.

RE: [PATCH 2/3] iwd: Connection of hidden networks

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

> I would really appreciate if you could send your patches following the
> normal Linux kernel patch submission work flow. That is send the patch
> directly with 'git send-email'. git send-email gets the threading etc
> all right. Please check the kernel docs on how to setup your git
> environment. Doing so I can pick them directly from the mailing list and
> don't have to manually copy&past stuff around.
Really? I thought you did not have anymore to copy and paste manually.
Sorry for the inconvenience!
Maybe you do not remember, but, unfortunately, my professional e-mail,
used to generate my git patches, does not allow SMTP, so I can not use
git send-email with it.
I will check if I can use a new external e-mail, maybe it will be easier for
everyone.

> In this case it would be better to set the pointer to NULL right after
> the resource is freed.
I don't think so, the resource is automatically freed by the GDBusDestroyFunction
argument function.

Best Regards,

Emmanuel