diff mbox series

[v5,02/17] mesh: factor out rsn initialization

Message ID 722cae36e2650c5c5e7c2b4749b4901c50c608a8.1527629631.git.peter.oh@bowerswilkins.com
State Changes Requested
Headers show
Series mesh: enable DFS channels in mesh mode | expand

Commit Message

Peter Oh May 29, 2018, 9:39 p.m. UTC 
From: Peter Oh <peter.oh@bowerswilkins.com>

RSN initialization can be used in different phases
if mesh initialization and mesh join don't happen
in sequence such as DFS CAC is done in between,
hence factor it out to help convering the case.

Signed-off-by: Peter Oh <peter.oh@bowerswilkins.com>
---
 wpa_supplicant/mesh.c | 84 ++++++++++++++++++++++++++++++---------------------
 wpa_supplicant/mesh.h |  1 +
 2 files changed, 50 insertions(+), 35 deletions(-)

Comments

Jouni Malinen May 31, 2018, 8:55 a.m. UTC | #1 
On Tue, May 29, 2018 at 02:39:06PM -0700, peter.oh@bowerswilkins.com wrote:
> RSN initialization can be used in different phases
> if mesh initialization and mesh join don't happen
> in sequence such as DFS CAC is done in between,
> hence factor it out to help convering the case.

There is only a single call to wpas_mesh_init_rsn() after this patch
series. Could you please clarify why this factoring out is done?

> diff --git a/wpa_supplicant/mesh.c b/wpa_supplicant/mesh.c
> @@ -147,6 +147,53 @@ static void wpas_mesh_copy_groups(struct hostapd_data *bss,
> +int wpas_mesh_init_rsn(struct wpa_supplicant *wpa_s)

And why would this function not be a static function since the only user
remains in mesh.c through this patch series?
Peter Oh May 31, 2018, 11:04 p.m. UTC | #2 
On 05/31/2018 01:55 AM, Jouni Malinen wrote:
> On Tue, May 29, 2018 at 02:39:06PM -0700, peter.oh@bowerswilkins.com wrote:
>> RSN initialization can be used in different phases
>> if mesh initialization and mesh join don't happen
>> in sequence such as DFS CAC is done in between,
>> hence factor it out to help convering the case.
> There is only a single call to wpas_mesh_init_rsn() after this patch
> series. Could you please clarify why this factoring out is done?
in the later patch, "use setup completion callback to complete mesh", 
this factored out function is moving to "wpas_mesh_complete_cb()"  which 
is not called before CAC is done, but only called after CAC is done. The 
original code which initializes RSN is a part of 
wpa_supplicant_mesh_init() that is called always regardless of CAC is 
done or not which leads auth failure when CAC is required.
>> diff --git a/wpa_supplicant/mesh.c b/wpa_supplicant/mesh.c
>> @@ -147,6 +147,53 @@ static void wpas_mesh_copy_groups(struct hostapd_data *bss,
>> +int wpas_mesh_init_rsn(struct wpa_supplicant *wpa_s)
> And why would this function not be a static function since the only user
> remains in mesh.c through this patch series?
Will update in next patchset.

Thanks,
Peter
Jouni Malinen June 11, 2018, 5:23 p.m. UTC | #3 
On Thu, May 31, 2018 at 04:04:33PM -0700, Peter Oh wrote:
> On 05/31/2018 01:55 AM, Jouni Malinen wrote:
> >There is only a single call to wpas_mesh_init_rsn() after this patch
> >series. Could you please clarify why this factoring out is done?
> in the later patch, "use setup completion callback to complete mesh", this
> factored out function is moving to "wpas_mesh_complete_cb()"  which is not
> called before CAC is done, but only called after CAC is done. The original
> code which initializes RSN is a part of wpa_supplicant_mesh_init() that is
> called always regardless of CAC is done or not which leads auth failure when
> CAC is required.

That does not really answer my question. All that happens here is in
patch 3/15 where wpas_mesh_init_rsn() call is moved from
wpa_supplicant_mesh_init() into wpas_join_mesh(). No other users for
this functionality is added. In practice, patches 2 and 3 are just
moving this functionality into another function in the same file.

The commit message on this patch 2/17 seems to imply that there might be
multiple places where RSN initialization could be used ("RSN
initialization can be used in different phases if mesh initialization
and mesh join don't happen in sequence such as DFS CAC is done in
between, hence factor it out to help convering the case."). What is that
commit message talking about? Where are the other "different phases"?
That description does not seem to agree with what this patch series does
and there does not seem to be any real need for factoring this
functionality out into a separate function. It might be reasonable to do
so from other reasons to keep the functions shorter, but if that is the
case, the commit message better say so. Now it is just making one wonder
why this change is being done and what other "phases" there might be
missing from this series.
diff mbox series

Patch

diff --git a/wpa_supplicant/mesh.c b/wpa_supplicant/mesh.c
index 87db8c5..9e46501 100644
--- a/wpa_supplicant/mesh.c
+++ b/wpa_supplicant/mesh.c
@@ -147,6 +147,53 @@  static void wpas_mesh_copy_groups(struct hostapd_data *bss,
 }
 
 
+int wpas_mesh_init_rsn(struct wpa_supplicant *wpa_s)
+{
+	struct hostapd_iface *ifmsh = wpa_s->ifmsh;
+	struct mesh_conf *mconf = wpa_s->ifmsh->mconf;
+	struct wpa_ssid *ssid = wpa_s->current_ssid;
+	struct hostapd_data *bss = ifmsh->bss[0];
+	static int default_groups[] = { 19, 20, 21, 25, 26, -1 };
+	const char *password;
+	size_t len;
+
+	if (mconf->security != MESH_CONF_SEC_NONE) {
+		password = ssid->sae_password;
+		if (!password)
+			password = ssid->passphrase;
+		if (!password) {
+			wpa_printf(MSG_ERROR,
+				   "mesh: Passphrase for SAE not configured");
+			return -1;
+		}
+
+		bss->conf->wpa = ssid->proto;
+		bss->conf->wpa_key_mgmt = ssid->key_mgmt;
+
+		if (wpa_s->conf->sae_groups &&
+		    wpa_s->conf->sae_groups[0] > 0) {
+			wpas_mesh_copy_groups(bss, wpa_s);
+		} else {
+			bss->conf->sae_groups =
+				os_memdup(default_groups,
+					  sizeof(default_groups));
+			if (!bss->conf->sae_groups)
+				return -1;
+		}
+
+		len = os_strlen(password);
+		bss->conf->ssid.wpa_passphrase =
+			dup_binstr(password, len);
+
+		wpa_s->mesh_rsn = mesh_rsn_auth_init(wpa_s, mconf);
+		if (!wpa_s->mesh_rsn)
+			return -1;
+	}
+
+	return 0;
+}
+
+
 static int wpa_supplicant_mesh_init(struct wpa_supplicant *wpa_s,
 				    struct wpa_ssid *ssid,
 				    struct hostapd_freq_params *freq)
@@ -156,9 +203,6 @@  static int wpa_supplicant_mesh_init(struct wpa_supplicant *wpa_s,
 	struct hostapd_config *conf;
 	struct mesh_conf *mconf;
 	int basic_rates_erp[] = { 10, 20, 55, 60, 110, 120, 240, -1 };
-	static int default_groups[] = { 19, 20, 21, 25, 26, -1 };
-	const char *password;
-	size_t len;
 	int rate_len;
 	int frequency;
 
@@ -292,38 +336,8 @@  static int wpa_supplicant_mesh_init(struct wpa_supplicant *wpa_s,
 		return -1;
 	}
 
-	if (mconf->security != MESH_CONF_SEC_NONE) {
-		password = ssid->sae_password;
-		if (!password)
-			password = ssid->passphrase;
-		if (!password) {
-			wpa_printf(MSG_ERROR,
-				   "mesh: Passphrase for SAE not configured");
-			goto out_free;
-		}
-
-		bss->conf->wpa = ssid->proto;
-		bss->conf->wpa_key_mgmt = ssid->key_mgmt;
-
-		if (wpa_s->conf->sae_groups &&
-		    wpa_s->conf->sae_groups[0] > 0) {
-			wpas_mesh_copy_groups(bss, wpa_s);
-		} else {
-			bss->conf->sae_groups =
-				os_memdup(default_groups,
-					  sizeof(default_groups));
-			if (!bss->conf->sae_groups)
-				goto out_free;
-		}
-
-		len = os_strlen(password);
-		bss->conf->ssid.wpa_passphrase =
-			dup_binstr(password, len);
-
-		wpa_s->mesh_rsn = mesh_rsn_auth_init(wpa_s, mconf);
-		if (!wpa_s->mesh_rsn)
-			goto out_free;
-	}
+	if (wpas_mesh_init_rsn(wpa_s))
+		goto out_free;
 
 	wpa_supplicant_conf_ap_ht(wpa_s, ssid, conf);
 
diff --git a/wpa_supplicant/mesh.h b/wpa_supplicant/mesh.h
index 2e2f3cf..9952102 100644
--- a/wpa_supplicant/mesh.h
+++ b/wpa_supplicant/mesh.h
@@ -22,6 +22,7 @@  int wpas_mesh_peer_remove(struct wpa_supplicant *wpa_s, const u8 *addr);
 int wpas_mesh_peer_add(struct wpa_supplicant *wpa_s, const u8 *addr,
 		       int duration);
 void wpas_join_mesh(struct wpa_supplicant *wpa_s);
+int wpas_mesh_init_rsn(struct wpa_supplicant *wpa_s);
 
 #ifdef CONFIG_MESH