* [PATCH 2/3] iwd: Connection of hidden networks
@ 2021-10-01 15:27 VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-04 7:03 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-10-01 15:27 UTC (permalink / raw)
To: connman
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-01 15:27 [PATCH 2/3] iwd: Connection of hidden networks VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-10-04 7:03 ` Daniel Wagner
2021-10-04 9:10 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2021-10-04 7:03 UTC (permalink / raw)
To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-04 7:03 ` Daniel Wagner
@ 2021-10-04 9:10 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-06 11:59 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-10-04 9:10 UTC (permalink / raw)
To: Daniel Wagner; +Cc: connman
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-04 9:10 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-10-06 11:59 ` Daniel Wagner
2021-10-06 12:44 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2021-10-06 11:59 UTC (permalink / raw)
To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-06 11:59 ` Daniel Wagner
@ 2021-10-06 12:44 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-06 12:56 ` Daniel Wagner
0 siblings, 1 reply; 7+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-10-06 12:44 UTC (permalink / raw)
To: Daniel Wagner; +Cc: connman
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-06 12:44 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-10-06 12:56 ` Daniel Wagner
2021-10-06 13:41 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Wagner @ 2021-10-06 12:56 UTC (permalink / raw)
To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3] iwd: Connection of hidden networks
2021-10-06 12:56 ` Daniel Wagner
@ 2021-10-06 13:41 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
0 siblings, 0 replies; 7+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-10-06 13:41 UTC (permalink / raw)
To: Daniel Wagner; +Cc: connman
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-06 13:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 15:27 [PATCH 2/3] iwd: Connection of hidden networks VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-04 7:03 ` Daniel Wagner
2021-10-04 9:10 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-06 11:59 ` Daniel Wagner
2021-10-06 12:44 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-10-06 12:56 ` Daniel Wagner
2021-10-06 13:41 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).