[PATCH 1/4] netdev: Replace bool randomize_mac with specific address

[PATCH 1/4] netdev: Replace bool randomize_mac with specific address

[Andrew Zaborowski]

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

Allow netdev_create_from_genl callers to draw a random or non-random MAC
and pass it in the parameter instead of a bool to tell us to generating
the MAC locally.  In P2P we are generating the MAC some time before
creating the netdev in order to pass it to the peer during negotiation.
---
 src/manager.c | 22 ++++++++++++++++++----
 src/netdev.c  | 45 ++++++++++++++++++++++-----------------------
 src/netdev.h  |  3 ++-
 3 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/src/manager.c b/src/manager.c
index 47874e11..4ad0f03c 100644
--- a/src/manager.c
+++ b/src/manager.c
@@ -102,6 +102,9 @@ static void wiphy_setup_state_destroy(struct wiphy_setup_state *state)
 
 static bool manager_use_default(struct wiphy_setup_state *state)
 {
+	uint8_t addr_buf[6];
+	uint8_t *addr = NULL;
+
 	l_debug("");
 
 	if (!state->default_if_msg) {
@@ -110,13 +113,20 @@ static bool manager_use_default(struct wiphy_setup_state *state)
 		return false;
 	}
 
-	netdev_create_from_genl(state->default_if_msg, randomize);
+	if (randomize) {
+		wiphy_generate_random_address(state->wiphy, addr_buf);
+		addr = addr_buf;
+	}
+
+	netdev_create_from_genl(state->default_if_msg, addr);
 	return true;
 }
 
 static void manager_new_interface_cb(struct l_genl_msg *msg, void *user_data)
 {
 	struct wiphy_setup_state *state = user_data;
+	uint8_t addr_buf[6];
+	uint8_t *addr = NULL;
 
 	l_debug("");
 
@@ -134,9 +144,13 @@ static void manager_new_interface_cb(struct l_genl_msg *msg, void *user_data)
 		return;
 	}
 
-	netdev_create_from_genl(msg, randomize &&
-					!wiphy_has_feature(state->wiphy,
-						NL80211_FEATURE_MAC_ON_CREATE));
+	if (randomize && !wiphy_has_feature(state->wiphy,
+					NL80211_FEATURE_MAC_ON_CREATE)) {
+		wiphy_generate_random_address(state->wiphy, addr_buf);
+		addr = addr_buf;
+	}
+
+	netdev_create_from_genl(msg, addr);
 }
 
 static void manager_new_interface_done(void *user_data)
diff --git a/src/netdev.c b/src/netdev.c
index 46c79fb0..77668151 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -125,6 +125,7 @@ struct netdev {
 	int8_t cur_rssi;
 	struct l_timeout *rssi_poll_timeout;
 	uint32_t rssi_poll_cmd_id;
+	uint8_t set_mac_once[6];
 
 	uint32_t set_powered_cmd_id;
 	netdev_command_cb_t set_powered_cb;
@@ -147,7 +148,6 @@ struct netdev {
 	bool ignore_connect_event : 1;
 	bool expect_connect_failure : 1;
 	bool aborting : 1;
-	bool mac_randomize_once : 1;
 	bool events_ready : 1;
 };
 
@@ -4259,6 +4259,20 @@ static void netdev_set_mac_cb(int error, uint16_t type, const void *data,
 					netdev_initial_up_cb, netdev, NULL);
 }
 
+static bool netdev_check_set_mac(struct netdev *netdev)
+{
+	if (util_mem_is_zero(netdev->set_mac_once, 6))
+		return false;
+
+	l_debug("Setting initial address on ifindex: %d to: " MAC,
+		netdev->index, MAC_STR(netdev->set_mac_once));
+	netdev->set_powered_cmd_id =
+		rtnl_set_mac(rtnl, netdev->index, netdev->set_mac_once,
+				netdev_set_mac_cb, netdev, NULL);
+	memset(netdev->set_mac_once, 0, 6);
+	return true;
+}
+
 static void netdev_initial_down_cb(int error, uint16_t type, const void *data,
 					uint32_t len, void *user_data)
 {
@@ -4274,17 +4288,8 @@ static void netdev_initial_down_cb(int error, uint16_t type, const void *data,
 		return;
 	}
 
-	if (netdev->mac_randomize_once) {
-		uint8_t addr[ETH_ALEN];
-
-		wiphy_generate_random_address(netdev->wiphy, addr);
-		l_debug("Setting initial random address on "
-			"ifindex: %d to: "MAC, netdev->index, MAC_STR(addr));
-		netdev->set_powered_cmd_id =
-			rtnl_set_mac(rtnl, netdev->index, addr,
-					netdev_set_mac_cb, netdev, NULL);
+	if (netdev_check_set_mac(netdev))
 		return;
-	}
 
 	netdev->set_powered_cmd_id =
 		rtnl_set_powered(rtnl, netdev->index, true,
@@ -4326,17 +4331,8 @@ static void netdev_getlink_cb(int error, uint16_t type, const void *data,
 	 */
 	powered = netdev_get_is_up(netdev);
 
-	if (!powered && netdev->mac_randomize_once) {
-		uint8_t addr[ETH_ALEN];
-
-		wiphy_generate_random_address(netdev->wiphy, addr);
-		l_debug("Setting initial random address on "
-			"ifindex: %d to: "MAC, netdev->index, MAC_STR(addr));
-		netdev->set_powered_cmd_id =
-			rtnl_set_mac(rtnl, netdev->index, addr,
-					netdev_set_mac_cb, netdev, NULL);
+	if (!powered && netdev_check_set_mac(netdev))
 		return;
-	}
 
 	cb = powered ? netdev_initial_down_cb : netdev_initial_up_cb;
 
@@ -4511,7 +4507,8 @@ error:
 	return NULL;
 }
 
-struct netdev *netdev_create_from_genl(struct l_genl_msg *msg, bool random_mac)
+struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
+					const uint8_t *set_mac)
 {
 	struct l_genl_attr attr;
 	uint16_t type, len;
@@ -4628,7 +4625,9 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg, bool random_mac)
 	memcpy(netdev->name, ifname, ifname_len);
 	netdev->wiphy = wiphy;
 	netdev->pae_over_nl80211 = pae_io == NULL;
-	netdev->mac_randomize_once = random_mac;
+
+	if (set_mac)
+		memcpy(netdev->set_mac_once, set_mac, 6);
 
 	if (pae_io) {
 		netdev->pae_io = pae_io;
diff --git a/src/netdev.h b/src/netdev.h
index 032265f9..192cb4fe 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -205,5 +205,6 @@ uint32_t netdev_station_watch_add(struct netdev *netdev,
 
 bool netdev_station_watch_remove(struct netdev *netdev, uint32_t id);
 
-struct netdev *netdev_create_from_genl(struct l_genl_msg *msg, bool random_mac);
+struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
+					const uint8_t *set_mac);
 bool netdev_destroy(struct netdev *netdev);
-- 
2.20.1

[PATCH 2/4] wsc: Refactor to separate station-specific code

[Andrew Zaborowski]

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

Split the interface-specific logic from the core WSC logic.  The core
WSC code is the part that we can re-use between P2P and station and
doesn't include the D-Bus code, scanning for the target BSS or the
attempt to make a station mode connection.
---
 src/wsc.c | 141 ++++++++++++++++++++++++++++++++++++++----------------
 src/wsc.h |   3 ++
 2 files changed, 103 insertions(+), 41 deletions(-)

diff --git a/src/wsc.c b/src/wsc.c
index eb8489f3..222cc222 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -65,6 +65,11 @@ struct wsc {
 	struct wsc_credentials_info creds[3];
 	uint32_t n_creds;
 	struct l_settings *eap_settings;
+	char *pin;
+
+	wsc_done_cb_t done_cb;
+	void *done_data;
+	uint16_t config_method;
 
 	bool wsc_association : 1;
 };
@@ -153,15 +158,12 @@ static void wsc_try_credentials(struct wsc *wsc)
 		l_dbus_message_unref(wsc->pending);
 		wsc->pending = NULL;
 
-		goto done;
+		return;
 	}
 
 	dbus_pending_reply(&wsc->pending,
 					wsc_error_not_reachable(wsc->pending));
 	station_set_autoconnect(wsc->station, true);
-done:
-	memset(wsc->creds, 0, sizeof(wsc->creds));
-	wsc->n_creds = 0;
 }
 
 static void wsc_store_credentials(struct wsc *wsc)
@@ -217,6 +219,22 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
 	station_set_autoconnect(wsc->station, true);
 }
 
+static void wsc_connect_cleanup(struct wsc *wsc)
+{
+	wsc->wsc_association = false;
+
+	l_settings_free(wsc->eap_settings);
+	wsc->eap_settings = NULL;
+
+	if (wsc->pin) {
+		explicit_bzero(wsc->pin, strlen(wsc->pin));
+		l_free(wsc->pin);
+		wsc->pin = NULL;
+	}
+
+	wsc->target = NULL;
+}
+
 static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 					void *event_data, void *user_data)
 {
@@ -224,33 +242,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 
 	l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
 
-	wsc->wsc_association = false;
-
-	l_settings_free(wsc->eap_settings);
-	wsc->eap_settings = NULL;
+	wsc_connect_cleanup(wsc);
 
 	if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
-		wsc_store_credentials(wsc);
-		wsc_try_credentials(wsc);
+		struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
+		uint32_t n_creds = wsc->n_creds;
+
+		memcpy(creds, wsc->creds, sizeof(creds));
+		explicit_bzero(wsc->creds, sizeof(creds));
+		wsc->n_creds = 0;
+		wsc->done_cb(0, creds, n_creds, wsc->done_data);
+		explicit_bzero(creds, sizeof(creds));
 		return;
 	}
 
 	switch (result) {
 	case NETDEV_RESULT_ABORTED:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_aborted(wsc->pending));
+		wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
 		return;
 	case NETDEV_RESULT_HANDSHAKE_FAILED:
-		dbus_pending_reply(&wsc->pending,
-					wsc_error_no_credentials(wsc->pending));
+		wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
 		break;
 	default:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_failed(wsc->pending));
+		wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
 		break;
 	}
 
-	station_set_autoconnect(wsc->station, true);
+	if (wsc->station)
+		station_set_autoconnect(wsc->station, true);
 }
 
 static void wsc_credential_obtained(struct wsc *wsc,
@@ -452,30 +471,24 @@ static void wsc_connect(struct wsc *wsc)
 	l_settings_set_uint(settings, "WSC", "RFBand",
 					freq_to_rf_band(bss->frequency));
 	l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
-				WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
-				WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
-				WSC_CONFIGURATION_METHOD_KEYPAD);
+					wsc->config_method);
 	l_settings_set_string(settings, "WSC", "PrimaryDeviceType",
 					"0-00000000-0");
 	l_settings_set_string(settings, "WSC", "EnrolleeMAC",
 		util_address_to_string(netdev_get_address(wsc->netdev)));
 
-	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
-		const char *pin;
-
-		if (l_dbus_message_get_arguments(wsc->pending, "s", &pin)) {
-			enum wsc_device_password_id dpid;
+	if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+		enum wsc_device_password_id dpid;
 
-			if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
-				dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
-			else
-				dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+		if (strlen(wsc->pin) == 4 ||
+				wsc_pin_is_checksum_valid(wsc->pin))
+			dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+		else
+			dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
 
-			l_settings_set_uint(settings, "WSC",
-						"DevicePasswordId", dpid);
-			l_settings_set_string(settings, "WSC",
-						"DevicePassword", pin);
-		}
+		l_settings_set_uint(settings, "WSC", "DevicePasswordId", dpid);
+		l_settings_set_string(settings, "WSC", "DevicePassword",
+					wsc->pin);
 	}
 
 	handshake_state_set_event_func(hs, wsc_handshake_event, wsc);
@@ -511,8 +524,41 @@ static void wsc_connect(struct wsc *wsc)
 	return;
 error:
 	handshake_state_free(hs);
-	dbus_pending_reply(&wsc->pending,
-				dbus_error_failed(wsc->pending));
+	wsc_connect_cleanup(wsc);
+	wsc->done_cb(r, NULL, 0, wsc->done_data);
+}
+
+/* Done callback for when WSC is triggered by DBus methods */
+static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
+				unsigned int n_creds, void *user_data)
+{
+	struct wsc *wsc = user_data;
+
+	l_debug("err=%i", err);
+
+	switch (err) {
+	case 0:
+		break;
+	case -ECANCELED:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_aborted(wsc->pending));
+		return;
+	case -ENOKEY:
+		dbus_pending_reply(&wsc->pending,
+					wsc_error_no_credentials(wsc->pending));
+		return;
+	case -EBUSY:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_busy(wsc->pending));
+		return;
+	default:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_failed(wsc->pending));
+		return;
+	}
+
+	wsc_store_credentials(wsc);
+	wsc_try_credentials(wsc);
 }
 
 static void station_state_watch(enum station_state state, void *userdata)
@@ -541,6 +587,22 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 	wsc->target = target;
 	station_set_autoconnect(wsc->station, false);
 
+	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
+		char *pin;
+
+		wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
+
+		if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
+			goto error;
+
+		wsc->pin = l_strdup(pin);
+	} else
+		wsc->config_method =
+			WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
+
+	wsc->done_cb = wsc_dbus_done_cb;
+	wsc->done_data = wsc;
+
 	switch (station_get_state(wsc->station)) {
 	case STATION_STATE_DISCONNECTED:
 		wsc_connect(wsc);
@@ -564,7 +626,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 		break;
 	}
 error:
-	wsc->target = NULL;
+	wsc_connect_cleanup(wsc);
 	dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
 }
 
@@ -1074,12 +1136,12 @@ static void wsc_free(void *userdata)
 	struct wsc *wsc = userdata;
 
 	wsc_cancel_scan(wsc);
+	wsc_connect_cleanup(wsc);
 
 	if (wsc->station_state_watch) {
 		station_remove_state_watch(wsc->station,
 						wsc->station_state_watch);
 		wsc->station_state_watch = 0;
-		wsc->target = NULL;
 	}
 
 	if (wsc->pending)
@@ -1090,9 +1152,6 @@ static void wsc_free(void *userdata)
 		dbus_pending_reply(&wsc->pending_cancel,
 				dbus_error_aborted(wsc->pending_cancel));
 
-	if (wsc->eap_settings)
-		l_settings_free(wsc->eap_settings);
-
 	l_free(wsc);
 }
 
diff --git a/src/wsc.h b/src/wsc.h
index b0e6f24a..ada08c89 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -30,3 +30,6 @@ struct wsc_credentials_info {
 	uint8_t addr[6];
 	bool has_passphrase;
 };
+
+typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
+				unsigned int n_creds, void *user_data);
-- 
2.20.1

Re: [PATCH 2/4] wsc: Refactor to separate station-specific code

[Denis Kenzior]

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

Hi Andrew,

On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
> Split the interface-specific logic from the core WSC logic.  The core
> WSC code is the part that we can re-use between P2P and station and
> doesn't include the D-Bus code, scanning for the target BSS or the
> attempt to make a station mode connection.
> ---
>   src/wsc.c | 141 ++++++++++++++++++++++++++++++++++++++----------------
>   src/wsc.h |   3 ++
>   2 files changed, 103 insertions(+), 41 deletions(-)
> 

<snip>

> @@ -224,33 +242,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
>   
>   	l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
>   
> -	wsc->wsc_association = false;
> -
> -	l_settings_free(wsc->eap_settings);
> -	wsc->eap_settings = NULL;
> +	wsc_connect_cleanup(wsc);
>   
>   	if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
> -		wsc_store_credentials(wsc);
> -		wsc_try_credentials(wsc);
> +		struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
> +		uint32_t n_creds = wsc->n_creds;
> +
> +		memcpy(creds, wsc->creds, sizeof(creds));
> +		explicit_bzero(wsc->creds, sizeof(creds));
> +		wsc->n_creds = 0;

I added a comment here based on your response to my previous question 
about this.  Please check to see that I got the gist of it correctly.

> +		wsc->done_cb(0, creds, n_creds, wsc->done_data);
> +		explicit_bzero(creds, sizeof(creds));
>   		return;
>   	}
>   

<snip>

> @@ -452,30 +471,24 @@ static void wsc_connect(struct wsc *wsc)
>   	l_settings_set_uint(settings, "WSC", "RFBand",
>   					freq_to_rf_band(bss->frequency));
>   	l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
> -				WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
> -				WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
> -				WSC_CONFIGURATION_METHOD_KEYPAD);
> +					wsc->config_method);

I squashed this for now.  I don't think it is actually correct and I 
brought this up previously.  ConfigurationMethods are supposed to be all 
the methods we support, not the selected one...  Why this is useful 
inside the EAP-WSC protocol is unclear, but that seems to be what the 
spec wants...

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/4] wsc: Refactor to separate station-specific code

[Andrew Zaborowski]

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

Hi Denis,

On Mon, 6 Jan 2020 at 18:55, Denis Kenzior <denkenz@gmail.com> wrote:
> On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
> > Split the interface-specific logic from the core WSC logic.  The core
> > WSC code is the part that we can re-use between P2P and station and
> > doesn't include the D-Bus code, scanning for the target BSS or the
> > attempt to make a station mode connection.
> > ---
> >   src/wsc.c | 141 ++++++++++++++++++++++++++++++++++++++----------------
> >   src/wsc.h |   3 ++
> >   2 files changed, 103 insertions(+), 41 deletions(-)
> >
>
> <snip>
>
> > @@ -224,33 +242,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
> >
> >       l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
> >
> > -     wsc->wsc_association = false;
> > -
> > -     l_settings_free(wsc->eap_settings);
> > -     wsc->eap_settings = NULL;
> > +     wsc_connect_cleanup(wsc);
> >
> >       if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
> > -             wsc_store_credentials(wsc);
> > -             wsc_try_credentials(wsc);
> > +             struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
> > +             uint32_t n_creds = wsc->n_creds;
> > +
> > +             memcpy(creds, wsc->creds, sizeof(creds));
> > +             explicit_bzero(wsc->creds, sizeof(creds));
> > +             wsc->n_creds = 0;
>
> I added a comment here based on your response to my previous question
> about this.  Please check to see that I got the gist of it correctly.

Ok, good idea.  The reason was basically that done_cb should be able
to free @wsc (and it would in my earlier p2p implementation), but we
needed to wipe the creds before freeing the memory so my only option
was to always wipe it.

> > +             wsc->done_cb(0, creds, n_creds, wsc->done_data);
> > +             explicit_bzero(creds, sizeof(creds));
> >               return;
> >       }
> >
>
> <snip>
>
> > @@ -452,30 +471,24 @@ static void wsc_connect(struct wsc *wsc)
> >       l_settings_set_uint(settings, "WSC", "RFBand",
> >                                       freq_to_rf_band(bss->frequency));
> >       l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
> > -                             WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
> > -                             WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
> > -                             WSC_CONFIGURATION_METHOD_KEYPAD);
> > +                                     wsc->config_method);
>
> I squashed this for now.  I don't think it is actually correct and I
> brought this up previously.  ConfigurationMethods are supposed to be all
> the methods we support, not the selected one...  Why this is useful
> inside the EAP-WSC protocol is unclear, but that seems to be what the
> spec wants...

Oops, I think you brought this up twice actually and I definitely
looked at the spec and dropped this change.  So I don't know how it
made it into the last iteration of this patch.

Best regards

[PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Andrew Zaborowski]

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

Allow adding the DBus SimpleConfiguration interface on both station mode
netdevs and P2P peers by putting public functions and callbacks between
the DBus method handlers and their actual implementation.  The
implementation provides its own callbacks for each method using the
wsc_ops struct.

For patch readability I didn't move some functions in wsc.c around and
group them logically, we can do the moving separately.
---
 src/wsc.c | 406 ++++++++++++++++++++++++++++++++----------------------
 src/wsc.h |  21 ++-
 2 files changed, 263 insertions(+), 164 deletions(-)

diff --git a/src/wsc.c b/src/wsc.c
index 222cc222..1901feb6 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -52,6 +52,9 @@
 static uint32_t netdev_watch = 0;
 
 struct wsc {
+	const struct wsc_ops *ops;
+	void *user_data;
+
 	struct netdev *netdev;
 	struct station *station;
 	struct l_dbus_message *pending;
@@ -66,9 +69,6 @@ struct wsc {
 	uint32_t n_creds;
 	struct l_settings *eap_settings;
 	char *pin;
-
-	wsc_done_cb_t done_cb;
-	void *done_data;
 	uint16_t config_method;
 
 	bool wsc_association : 1;
@@ -113,20 +113,20 @@ static struct l_dbus_message *wsc_error_time_expired(struct l_dbus_message *msg)
 					"No APs in PIN mode found in "
 					"the alloted time");
 }
-static void wsc_try_credentials(struct wsc *wsc)
+static void wsc_try_credentials(struct wsc *wsc, struct wsc_credentials_info *creds,
+					unsigned int n_creds)
 {
 	unsigned int i;
 	struct network *network;
 	struct scan_bss *bss;
 
-	for (i = 0; i < wsc->n_creds; i++) {
-		network = station_network_find(wsc->station,
-						wsc->creds[i].ssid,
-						wsc->creds[i].security);
+	for (i = 0; i < n_creds; i++) {
+		network = station_network_find(wsc->station, creds[i].ssid,
+						creds[i].security);
 		if (!network)
 			continue;
 
-		bss = network_bss_find_by_addr(network, wsc->creds[i].addr);
+		bss = network_bss_find_by_addr(network, creds[i].addr);
 
 		if (!bss)
 			bss = network_bss_select(network, true);
@@ -134,7 +134,7 @@ static void wsc_try_credentials(struct wsc *wsc)
 		if (!bss)
 			continue;
 
-		if (wsc->creds[i].security == SECURITY_PSK) {
+		if (creds[i].security == SECURITY_PSK) {
 			bool ret;
 
 			/*
@@ -142,12 +142,11 @@ static void wsc_try_credentials(struct wsc *wsc)
 			 * WPA2 and WPA3 since the PSK can always be generated
 			 * if needed
 			 */
-			if (wsc->creds[i].has_passphrase)
+			if (creds[i].has_passphrase)
 				ret = network_set_passphrase(network,
-						wsc->creds[i].passphrase);
+							creds[i].passphrase);
 			else
-				ret = network_set_psk(network,
-						wsc->creds[i].psk);
+				ret = network_set_psk(network, creds[i].psk);
 
 			if (!ret)
 				continue;
@@ -166,21 +165,22 @@ static void wsc_try_credentials(struct wsc *wsc)
 	station_set_autoconnect(wsc->station, true);
 }
 
-static void wsc_store_credentials(struct wsc *wsc)
+static void wsc_store_credentials(struct wsc_credentials_info *creds,
+					unsigned int n_creds)
 {
 	unsigned int i;
 
-	for (i = 0; i < wsc->n_creds; i++) {
-		enum security security = wsc->creds[i].security;
-		const char *ssid = wsc->creds[i].ssid;
+	for (i = 0; i < n_creds; i++) {
+		enum security security = creds[i].security;
+		const char *ssid = creds[i].ssid;
 		struct l_settings *settings = l_settings_new();
 
 		l_debug("Storing credential for '%s(%s)'", ssid,
 						security_to_str(security));
 
 		if (security == SECURITY_PSK) {
-			char *hex = l_util_hexstring(wsc->creds[i].psk,
-						sizeof(wsc->creds[i].psk));
+			char *hex = l_util_hexstring(creds[i].psk,
+						sizeof(creds[i].psk));
 
 			l_settings_set_value(settings, "Security",
 							"PreSharedKey", hex);
@@ -210,8 +210,6 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
 
 	l_debug("%p, success: %d", wsc, success);
 
-	wsc->wsc_association = false;
-
 	reply = l_dbus_message_new_method_return(wsc->pending_cancel);
 	l_dbus_message_set_arguments(reply, "");
 	dbus_pending_reply(&wsc->pending_cancel, reply);
@@ -251,20 +249,20 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 		memcpy(creds, wsc->creds, sizeof(creds));
 		explicit_bzero(wsc->creds, sizeof(creds));
 		wsc->n_creds = 0;
-		wsc->done_cb(0, creds, n_creds, wsc->done_data);
+		wsc->ops->enrollee_done_cb(0, creds, n_creds, wsc->user_data);
 		explicit_bzero(creds, sizeof(creds));
 		return;
 	}
 
 	switch (result) {
 	case NETDEV_RESULT_ABORTED:
-		wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
+		wsc->ops->enrollee_done_cb(-ECANCELED, NULL, 0, wsc->user_data);
 		return;
 	case NETDEV_RESULT_HANDSHAKE_FAILED:
-		wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
+		wsc->ops->enrollee_done_cb(-ENOKEY, NULL, 0, wsc->user_data);
 		break;
 	default:
-		wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
+		wsc->ops->enrollee_done_cb(-EIO, NULL, 0, wsc->user_data);
 		break;
 	}
 
@@ -525,40 +523,19 @@ static void wsc_connect(struct wsc *wsc)
 error:
 	handshake_state_free(hs);
 	wsc_connect_cleanup(wsc);
-	wsc->done_cb(r, NULL, 0, wsc->done_data);
+	wsc->ops->enrollee_done_cb(r, NULL, 0, wsc->user_data);
 }
 
-/* Done callback for when WSC is triggered by DBus methods */
-static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
-				unsigned int n_creds, void *user_data)
+static void wsc_connect_set_method(struct wsc *wsc)
 {
-	struct wsc *wsc = user_data;
-
-	l_debug("err=%i", err);
+	const char *pin;
 
-	switch (err) {
-	case 0:
-		break;
-	case -ECANCELED:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_aborted(wsc->pending));
-		return;
-	case -ENOKEY:
-		dbus_pending_reply(&wsc->pending,
-					wsc_error_no_credentials(wsc->pending));
-		return;
-	case -EBUSY:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_busy(wsc->pending));
-		return;
-	default:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_failed(wsc->pending));
-		return;
-	}
+	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
+		l_dbus_message_get_arguments(wsc->pending, "s", &pin);
+	else
+		pin = NULL;
 
-	wsc_store_credentials(wsc);
-	wsc_try_credentials(wsc);
+	wsc_start_enrollee(wsc, wsc->netdev, wsc->target, pin, NULL, 0);
 }
 
 static void station_state_watch(enum station_state state, void *userdata)
@@ -573,7 +550,7 @@ static void station_state_watch(enum station_state state, void *userdata)
 	station_remove_state_watch(wsc->station, wsc->station_state_watch);
 	wsc->station_state_watch = 0;
 
-	wsc_connect(wsc);
+	wsc_connect_set_method(wsc);
 }
 
 static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
@@ -587,25 +564,9 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 	wsc->target = target;
 	station_set_autoconnect(wsc->station, false);
 
-	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
-		char *pin;
-
-		wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
-
-		if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
-			goto error;
-
-		wsc->pin = l_strdup(pin);
-	} else
-		wsc->config_method =
-			WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
-
-	wsc->done_cb = wsc_dbus_done_cb;
-	wsc->done_data = wsc;
-
 	switch (station_get_state(wsc->station)) {
 	case STATION_STATE_DISCONNECTED:
-		wsc_connect(wsc);
+		wsc_connect_set_method(wsc);
 		return;
 	case STATION_STATE_CONNECTING:
 	case STATION_STATE_CONNECTED:
@@ -652,20 +613,13 @@ static void walk_timeout(struct l_timeout *timeout, void *user_data)
 
 	wsc_cancel_scan(wsc);
 
-	if (wsc->pending)
-		dbus_pending_reply(&wsc->pending,
-				wsc_error_walk_time_expired(wsc->pending));
-}
+	if (wsc->pending) {
+		struct l_dbus_message *reply = wsc->pin ?
+			wsc_error_time_expired(wsc->pending) :
+			wsc_error_walk_time_expired(wsc->pending);
 
-static void pin_timeout(struct l_timeout *timeout, void *user_data)
-{
-	struct wsc *wsc = user_data;
-
-	wsc_cancel_scan(wsc);
-
-	if (wsc->pending)
-		dbus_pending_reply(&wsc->pending,
-					wsc_error_time_expired(wsc->pending));
+		dbus_pending_reply(&wsc->pending, reply);
+	}
 }
 
 static bool push_button_scan_results(int err, struct l_queue *bss_list,
@@ -1001,20 +955,7 @@ static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
 
 	l_debug("");
 
-	if (wsc->pending)
-		return dbus_error_busy(message);
-
-	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
-	if (!wsc->station)
-		return dbus_error_not_available(message);
-
-	if (!wsc_initiate_scan(wsc, WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON,
-				push_button_scan_results))
-		return dbus_error_failed(message);
-
-	wsc->walk_timer = l_timeout_create(WALK_TIME, walk_timeout, wsc, NULL);
-	wsc->pending = l_dbus_message_ref(message);
-
+	wsc->ops->connect(message, NULL, wsc->user_data);
 	return NULL;
 }
 
@@ -1046,77 +987,29 @@ static struct l_dbus_message *wsc_start_pin(struct l_dbus *dbus,
 {
 	struct wsc *wsc = user_data;
 	const char *pin;
-	enum wsc_device_password_id dpid;
 
 	l_debug("");
 
-	if (wsc->pending)
-		return dbus_error_busy(message);
-
-	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
-	if (!wsc->station)
-		return dbus_error_not_available(message);
-
 	if (!l_dbus_message_get_arguments(message, "s", &pin))
 		return dbus_error_invalid_args(message);
 
 	if (!wsc_pin_is_valid(pin))
 		return dbus_error_invalid_format(message);
 
-	if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
-		dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
-	else
-		dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
-
-	if (!wsc_initiate_scan(wsc, dpid, pin_scan_results))
-		return dbus_error_failed(message);
-
-	wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
-	wsc->pending = l_dbus_message_ref(message);
-
+	wsc->ops->connect(message, pin, wsc->user_data);
 	return NULL;
 }
 
-static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
+static struct l_dbus_message *wsc_dbus_cancel(struct l_dbus *dbus,
 						struct l_dbus_message *message,
 						void *user_data)
 {
 	struct wsc *wsc = user_data;
-	struct l_dbus_message *reply;
 
 	l_debug("");
 
-	if (!wsc->pending)
-		return dbus_error_not_available(message);
-
-	wsc_cancel_scan(wsc);
-
-	if (wsc->station_state_watch) {
-		station_remove_state_watch(wsc->station,
-						wsc->station_state_watch);
-		wsc->station_state_watch = 0;
-		wsc->target = NULL;
-	}
-
-	if (wsc->wsc_association) {
-		int r;
-
-		r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
-		if (r == 0) {
-			wsc->pending_cancel = l_dbus_message_ref(message);
-			return NULL;
-		}
-
-		l_warn("Unable to initiate disconnect: %s", strerror(-r));
-		wsc->wsc_association = false;
-	}
-
-	dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
-
-	reply = l_dbus_message_new_method_return(message);
-	l_dbus_message_set_arguments(reply, "");
-
-	return reply;
+	wsc->ops->cancel(message, wsc->user_data);
+	return NULL;
 }
 
 static void setup_wsc_interface(struct l_dbus_interface *interface)
@@ -1128,7 +1021,7 @@ static void setup_wsc_interface(struct l_dbus_interface *interface)
 	l_dbus_interface_method(interface, "StartPin", 0,
 				wsc_start_pin, "", "s", "pin");
 	l_dbus_interface_method(interface, "Cancel", 0,
-				wsc_cancel, "", "");
+				wsc_dbus_cancel, "", "");
 }
 
 static void wsc_free(void *userdata)
@@ -1155,7 +1048,194 @@ static void wsc_free(void *userdata)
 	l_free(wsc);
 }
 
-static void wsc_add_interface(struct netdev *netdev)
+struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data)
+{
+	struct l_dbus *dbus = dbus_get_bus();
+	struct wsc *wsc;
+	const char *path;
+
+	wsc = l_new(struct wsc, 1);
+	wsc->ops = ops;
+	wsc->user_data = user_data;
+
+	path = wsc->ops->get_path(wsc->user_data);
+	if (!path)
+		return wsc;
+
+	if (l_dbus_object_add_interface(dbus, path, IWD_WSC_INTERFACE, wsc))
+		return wsc;
+
+	wsc_free(wsc);
+	l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
+		path);
+	return NULL;
+}
+
+void wsc_destroy(struct wsc *wsc)
+{
+	l_dbus_object_remove_interface(dbus_get_bus(),
+					wsc->ops->get_path(wsc->user_data),
+					IWD_WSC_INTERFACE);
+}
+
+void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
+			struct scan_bss *target, const char *pin)
+{
+	wsc->netdev = netdev;
+	wsc->target = target;
+	wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
+		WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
+	wsc->pin = l_strdup(pin);
+
+	wsc_connect(wsc);
+}
+
+void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb)
+{
+	int r;
+
+	if (!wsc->wsc_association)
+		return;
+
+	r = netdev_disconnect(wsc->netdev, cb, wsc->user_data);
+	if (r == 0)
+		return;
+
+	l_warn("Unable to initiate disconnect: %s", strerror(-r));
+	wsc_connect_cleanup(wsc);
+	wsc->ops->enrollee_done_cb(-ECANCELED, NULL, 0, wsc->user_data);
+
+	if (cb)
+		cb(wsc->netdev, false, wsc->user_data);
+}
+
+static const char *wsc_device_get_path(void *user_data)
+{
+	struct wsc *wsc = user_data;
+
+	if (!wsc || !wsc->netdev)
+		return NULL;
+
+	return netdev_get_path(wsc->netdev);
+}
+
+static void wsc_device_connect(struct l_dbus_message *message, const char *pin,
+				void *user_data)
+{
+	struct wsc *wsc = user_data;
+	struct l_dbus *dbus = dbus_get_bus();
+	unsigned int walk_time;
+	enum wsc_device_password_id dpid;
+	scan_notify_func_t results_cb;
+
+	if (wsc->pending) {
+		l_dbus_send(dbus, dbus_error_busy(message));
+		return;
+	}
+
+	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
+	if (!wsc->station) {
+		l_dbus_send(dbus, dbus_error_not_available(message));
+		return;
+	}
+
+	if (pin) {
+		if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
+			dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+		else
+			dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+
+		walk_time = 60;
+		results_cb = pin_scan_results;
+	} else {
+		dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
+		walk_time = WALK_TIME;
+		results_cb = push_button_scan_results;
+	}
+
+	if (!wsc_initiate_scan(wsc, dpid, results_cb)) {
+		l_dbus_send(dbus, dbus_error_failed(message));
+		return;
+	}
+
+	wsc->walk_timer = l_timeout_create(walk_time, walk_timeout, wsc, NULL);
+	wsc->pending = l_dbus_message_ref(message);
+}
+
+static void wsc_device_cancel(struct l_dbus_message *message, void *user_data)
+{
+	struct wsc *wsc = user_data;
+	struct l_dbus *dbus = dbus_get_bus();
+
+	if (!wsc->pending) {
+		l_dbus_send(dbus, dbus_error_not_available(message));
+		return;
+	}
+
+	if (wsc->pending_cancel) {
+		l_dbus_send(dbus, dbus_error_busy(message));
+		return;
+	}
+
+	wsc_cancel_scan(wsc);
+
+	if (wsc->station_state_watch) {
+		station_remove_state_watch(wsc->station,
+						wsc->station_state_watch);
+		wsc->station_state_watch = 0;
+		wsc->target = NULL;
+	}
+
+	if (wsc->wsc_association) {
+		wsc->pending_cancel = l_dbus_message_ref(message);
+		wsc_cancel(wsc, wsc_disconnect_cb);
+		return;
+	}
+
+	dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
+	l_dbus_send(dbus, l_dbus_message_new_method_return(message));
+}
+
+static void wsc_device_enrollee_done(int err, struct wsc_credentials_info *creds,
+					unsigned int n_creds, void *user_data)
+{
+	struct wsc *wsc = user_data;
+
+	l_debug("err=%i", err);
+
+	switch (err) {
+	case 0:
+		break;
+	case -ECANCELED:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_aborted(wsc->pending));
+		return;
+	case -ENOKEY:
+		dbus_pending_reply(&wsc->pending,
+					wsc_error_no_credentials(wsc->pending));
+		return;
+	case -EBUSY:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_busy(wsc->pending));
+		return;
+	default:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_failed(wsc->pending));
+		return;
+	}
+
+	wsc_store_credentials(creds, n_creds);
+	wsc_try_credentials(wsc, creds, n_creds);
+}
+
+static const struct wsc_ops wsc_device_ops = {
+	.get_path = wsc_device_get_path,
+	.connect = wsc_device_connect,
+	.cancel = wsc_device_cancel,
+	.enrollee_done_cb = wsc_device_enrollee_done,
+};
+
+static void wsc_device_new(struct netdev *netdev)
 {
 	struct l_dbus *dbus = dbus_get_bus();
 	struct wsc *wsc;
@@ -1167,15 +1247,17 @@ static void wsc_add_interface(struct netdev *netdev)
 		return;
 	}
 
-	wsc = l_new(struct wsc, 1);
+	wsc = wsc_new(&wsc_device_ops, NULL);
+	wsc->user_data = wsc;
 	wsc->netdev = netdev;
 
-	if (!l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
-						IWD_WSC_INTERFACE,
-						wsc)) {
-		wsc_free(wsc);
-		l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
-	}
+	if (l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
+					IWD_WSC_INTERFACE, wsc))
+		return;
+
+	wsc_free(wsc);
+	l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
+		netdev_get_path(netdev));
 }
 
 static void wsc_remove_interface(struct netdev *netdev)
@@ -1194,7 +1276,7 @@ static void wsc_netdev_watch(struct netdev *netdev,
 	case NETDEV_WATCH_EVENT_NEW:
 		if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION &&
 				netdev_get_is_up(netdev))
-			wsc_add_interface(netdev);
+			wsc_device_new(netdev);
 		break;
 	case NETDEV_WATCH_EVENT_DOWN:
 	case NETDEV_WATCH_EVENT_DEL:
diff --git a/src/wsc.h b/src/wsc.h
index ada08c89..e8a965f2 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -20,6 +20,11 @@
  *
  */
 
+struct wsc;
+struct netdev;
+struct scan_bss;
+struct iovec;
+
 struct wsc_credentials_info {
 	char ssid[33];
 	enum security security;
@@ -31,5 +36,17 @@ struct wsc_credentials_info {
 	bool has_passphrase;
 };
 
-typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
-				unsigned int n_creds, void *user_data);
+struct wsc_ops {
+	const char *(*get_path)(void *user_data);
+	void (*connect)(struct l_dbus_message *message, const char *pin,
+			void *user_data);
+	void (*cancel)(struct l_dbus_message *message, void *user_data);
+	void (*enrollee_done_cb)(int err, struct wsc_credentials_info *creds,
+					unsigned int n_creds, void *user_data);
+};
+
+struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data);
+void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
+			struct scan_bss *target, const char *pin);
+void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb);
+void wsc_destroy(struct wsc *wsc);
-- 
2.20.1

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Denis Kenzior]

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

Hi Andrew,

On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
> Allow adding the DBus SimpleConfiguration interface on both station mode
> netdevs and P2P peers by putting public functions and callbacks between
> the DBus method handlers and their actual implementation.  The
> implementation provides its own callbacks for each method using the
> wsc_ops struct.
> 
> For patch readability I didn't move some functions in wsc.c around and
> group them logically, we can do the moving separately.
> ---
>   src/wsc.c | 406 ++++++++++++++++++++++++++++++++----------------------
>   src/wsc.h |  21 ++-
>   2 files changed, 263 insertions(+), 164 deletions(-)
> 

<snip>

> @@ -113,20 +113,20 @@ static struct l_dbus_message *wsc_error_time_expired(struct l_dbus_message *msg)
>   					"No APs in PIN mode found in "
>   					"the alloted time");
>   }
> -static void wsc_try_credentials(struct wsc *wsc)
> +static void wsc_try_credentials(struct wsc *wsc, struct wsc_credentials_info *creds,
> +					unsigned int n_creds)
>   {
>   	unsigned int i;
>   	struct network *network;
>   	struct scan_bss *bss;
>   
> -	for (i = 0; i < wsc->n_creds; i++) {
> -		network = station_network_find(wsc->station,
> -						wsc->creds[i].ssid,
> -						wsc->creds[i].security);
> +	for (i = 0; i < n_creds; i++) {
> +		network = station_network_find(wsc->station, creds[i].ssid,
> +						creds[i].security);
>   		if (!network)
>   			continue;
>   
> -		bss = network_bss_find_by_addr(network, wsc->creds[i].addr);
> +		bss = network_bss_find_by_addr(network, creds[i].addr);
>   
>   		if (!bss)
>   			bss = network_bss_select(network, true);
> @@ -134,7 +134,7 @@ static void wsc_try_credentials(struct wsc *wsc)
>   		if (!bss)
>   			continue;
>   
> -		if (wsc->creds[i].security == SECURITY_PSK) {
> +		if (creds[i].security == SECURITY_PSK) {
>   			bool ret;
>   
>   			/*
> @@ -142,12 +142,11 @@ static void wsc_try_credentials(struct wsc *wsc)
>   			 * WPA2 and WPA3 since the PSK can always be generated
>   			 * if needed
>   			 */
> -			if (wsc->creds[i].has_passphrase)
> +			if (creds[i].has_passphrase)
>   				ret = network_set_passphrase(network,
> -						wsc->creds[i].passphrase);
> +							creds[i].passphrase);
>   			else
> -				ret = network_set_psk(network,
> -						wsc->creds[i].psk);
> +				ret = network_set_psk(network, creds[i].psk);
>   
>   			if (!ret)
>   				continue;
> @@ -166,21 +165,22 @@ static void wsc_try_credentials(struct wsc *wsc)
>   	station_set_autoconnect(wsc->station, true);
>   }
>   
> -static void wsc_store_credentials(struct wsc *wsc)
> +static void wsc_store_credentials(struct wsc_credentials_info *creds,
> +					unsigned int n_creds)
>   {
>   	unsigned int i;
>   
> -	for (i = 0; i < wsc->n_creds; i++) {
> -		enum security security = wsc->creds[i].security;
> -		const char *ssid = wsc->creds[i].ssid;
> +	for (i = 0; i < n_creds; i++) {
> +		enum security security = creds[i].security;
> +		const char *ssid = creds[i].ssid;
>   		struct l_settings *settings = l_settings_new();
>   
>   		l_debug("Storing credential for '%s(%s)'", ssid,
>   						security_to_str(security));
>   
>   		if (security == SECURITY_PSK) {
> -			char *hex = l_util_hexstring(wsc->creds[i].psk,
> -						sizeof(wsc->creds[i].psk));
> +			char *hex = l_util_hexstring(creds[i].psk,
> +						sizeof(creds[i].psk));
>   
>   			l_settings_set_value(settings, "Security",
>   							"PreSharedKey", hex);

I cherry-picked these two chunks as a separate commit.

> @@ -210,8 +210,6 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
>   
>   	l_debug("%p, success: %d", wsc, success);
>   
> -	wsc->wsc_association = false;
> -
>   	reply = l_dbus_message_new_method_return(wsc->pending_cancel);
>   	l_dbus_message_set_arguments(reply, "");
>   	dbus_pending_reply(&wsc->pending_cancel, reply);

Is this chunk a straggler from some earlier refactoring?  If so, then a 
separate patch would be desirable

<snip>

> @@ -525,40 +523,19 @@ static void wsc_connect(struct wsc *wsc)
>   error:
>   	handshake_state_free(hs);
>   	wsc_connect_cleanup(wsc);
> -	wsc->done_cb(r, NULL, 0, wsc->done_data);
> +	wsc->ops->enrollee_done_cb(r, NULL, 0, wsc->user_data);
>   }
>   
> -/* Done callback for when WSC is triggered by DBus methods */
> -static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
> -				unsigned int n_creds, void *user_data)
> +static void wsc_connect_set_method(struct wsc *wsc)
>   {
> -	struct wsc *wsc = user_data;
> -
> -	l_debug("err=%i", err);
> +	const char *pin;
>   
> -	switch (err) {
> -	case 0:
> -		break;
> -	case -ECANCELED:
> -		dbus_pending_reply(&wsc->pending,
> -					dbus_error_aborted(wsc->pending));
> -		return;
> -	case -ENOKEY:
> -		dbus_pending_reply(&wsc->pending,
> -					wsc_error_no_credentials(wsc->pending));
> -		return;
> -	case -EBUSY:
> -		dbus_pending_reply(&wsc->pending,
> -					dbus_error_busy(wsc->pending));
> -		return;
> -	default:
> -		dbus_pending_reply(&wsc->pending,
> -					dbus_error_failed(wsc->pending));
> -		return;
> -	}
> +	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
> +		l_dbus_message_get_arguments(wsc->pending, "s", &pin);
> +	else
> +		pin = NULL;
>   
> -	wsc_store_credentials(wsc);
> -	wsc_try_credentials(wsc);
> +	wsc_start_enrollee(wsc, wsc->netdev, wsc->target, pin, NULL, 0);

Is wsc_start_enrollee going to be called from p2p directly or only via 
the D-Bus API?  This enclosing function (wsc_connect_set_method) is now 
quite strange looking.  Maybe it'd look nicer to just set wsc->pin from 
inside the StartPin / PushButton handlers instead and avoid this completely?

>   }
>   
>   static void station_state_watch(enum station_state state, void *userdata)
> @@ -573,7 +550,7 @@ static void station_state_watch(enum station_state state, void *userdata)
>   	station_remove_state_watch(wsc->station, wsc->station_state_watch);
>   	wsc->station_state_watch = 0;
>   
> -	wsc_connect(wsc);
> +	wsc_connect_set_method(wsc);
>   }
>   
>   static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
> @@ -587,25 +564,9 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
>   	wsc->target = target;
>   	station_set_autoconnect(wsc->station, false);
>   
> -	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
> -		char *pin;
> -
> -		wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
> -
> -		if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
> -			goto error;
> -
> -		wsc->pin = l_strdup(pin);
> -	} else
> -		wsc->config_method =
> -			WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
> -
> -	wsc->done_cb = wsc_dbus_done_cb;
> -	wsc->done_data = wsc;
> -
>   	switch (station_get_state(wsc->station)) {
>   	case STATION_STATE_DISCONNECTED:
> -		wsc_connect(wsc);
> +		wsc_connect_set_method(wsc);
>   		return;
>   	case STATION_STATE_CONNECTING:
>   	case STATION_STATE_CONNECTED:
> @@ -652,20 +613,13 @@ static void walk_timeout(struct l_timeout *timeout, void *user_data)
>   
>   	wsc_cancel_scan(wsc);
>   
> -	if (wsc->pending)
> -		dbus_pending_reply(&wsc->pending,
> -				wsc_error_walk_time_expired(wsc->pending));
> -}
> +	if (wsc->pending) {
> +		struct l_dbus_message *reply = wsc->pin ?
> +			wsc_error_time_expired(wsc->pending) :
> +			wsc_error_walk_time_expired(wsc->pending);
>   
> -static void pin_timeout(struct l_timeout *timeout, void *user_data)
> -{
> -	struct wsc *wsc = user_data;
> -
> -	wsc_cancel_scan(wsc);
> -
> -	if (wsc->pending)
> -		dbus_pending_reply(&wsc->pending,
> -					wsc_error_time_expired(wsc->pending));
> +		dbus_pending_reply(&wsc->pending, reply);
> +	}
>   }
>   

Okay, but this really belongs as a separate patch

>   static bool push_button_scan_results(int err, struct l_queue *bss_list,
> @@ -1001,20 +955,7 @@ static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
>   
>   	l_debug("");
>   
> -	if (wsc->pending)
> -		return dbus_error_busy(message);
> -

So why would you move this check out into the ops provided operation and 
not keep it here?

> -	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
> -	if (!wsc->station)
> -		return dbus_error_not_available(message);
> -
> -	if (!wsc_initiate_scan(wsc, WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON,
> -				push_button_scan_results))
> -		return dbus_error_failed(message);
> -
> -	wsc->walk_timer = l_timeout_create(WALK_TIME, walk_timeout, wsc, NULL);
> -	wsc->pending = l_dbus_message_ref(message);
> -

Pending tracking should also likely be managed here...

> +	wsc->ops->connect(message, NULL, wsc->user_data);
>   	return NULL;
>   }
>   
> @@ -1046,77 +987,29 @@ static struct l_dbus_message *wsc_start_pin(struct l_dbus *dbus,
>   {
>   	struct wsc *wsc = user_data;
>   	const char *pin;
> -	enum wsc_device_password_id dpid;
>   
>   	l_debug("");
>   
> -	if (wsc->pending)
> -		return dbus_error_busy(message);

Same comments as above apply here ...

> -
> -	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
> -	if (!wsc->station)
> -		return dbus_error_not_available(message);
> -
>   	if (!l_dbus_message_get_arguments(message, "s", &pin))
>   		return dbus_error_invalid_args(message);
>   
>   	if (!wsc_pin_is_valid(pin))
>   		return dbus_error_invalid_format(message);
>   
> -	if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
> -		dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
> -	else
> -		dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
> -
> -	if (!wsc_initiate_scan(wsc, dpid, pin_scan_results))
> -		return dbus_error_failed(message);
> -
> -	wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
> -	wsc->pending = l_dbus_message_ref(message);
> -
> +	wsc->ops->connect(message, pin, wsc->user_data);
>   	return NULL;
>   }
>   
> -static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
> +static struct l_dbus_message *wsc_dbus_cancel(struct l_dbus *dbus,
>   						struct l_dbus_message *message,
>   						void *user_data)
>   {
>   	struct wsc *wsc = user_data;
> -	struct l_dbus_message *reply;
>   
>   	l_debug("");
>   
> -	if (!wsc->pending)
> -		return dbus_error_not_available(message);
> -
> -	wsc_cancel_scan(wsc);
> -
> -	if (wsc->station_state_watch) {
> -		station_remove_state_watch(wsc->station,
> -						wsc->station_state_watch);
> -		wsc->station_state_watch = 0;
> -		wsc->target = NULL;
> -	}
> -
> -	if (wsc->wsc_association) {
> -		int r;
> -
> -		r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
> -		if (r == 0) {
> -			wsc->pending_cancel = l_dbus_message_ref(message);
> -			return NULL;
> -		}
> -
> -		l_warn("Unable to initiate disconnect: %s", strerror(-r));
> -		wsc->wsc_association = false;
> -	}
> -
> -	dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
> -
> -	reply = l_dbus_message_new_method_return(message);
> -	l_dbus_message_set_arguments(reply, "");
> -
> -	return reply;
> +	wsc->ops->cancel(message, wsc->user_data);
> +	return NULL;
>   }
>   
>   static void setup_wsc_interface(struct l_dbus_interface *interface)
> @@ -1128,7 +1021,7 @@ static void setup_wsc_interface(struct l_dbus_interface *interface)
>   	l_dbus_interface_method(interface, "StartPin", 0,
>   				wsc_start_pin, "", "s", "pin");
>   	l_dbus_interface_method(interface, "Cancel", 0,
> -				wsc_cancel, "", "");
> +				wsc_dbus_cancel, "", "");
>   }
>   
>   static void wsc_free(void *userdata)
> @@ -1155,7 +1048,194 @@ static void wsc_free(void *userdata)
>   	l_free(wsc);
>   }
>   
> -static void wsc_add_interface(struct netdev *netdev)
> +struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data)
> +{
> +	struct l_dbus *dbus = dbus_get_bus();
> +	struct wsc *wsc;
> +	const char *path;
> +
> +	wsc = l_new(struct wsc, 1);
> +	wsc->ops = ops;
> +	wsc->user_data = user_data;

This part seems rather weird

> +
> +	path = wsc->ops->get_path(wsc->user_data);
> +	if (!path)
> +		return wsc;
> +
> +	if (l_dbus_object_add_interface(dbus, path, IWD_WSC_INTERFACE, wsc))
> +		return wsc;
> +
> +	wsc_free(wsc);
> +	l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
> +		path);
> +	return NULL;
> +}
> +
> +void wsc_destroy(struct wsc *wsc)
> +{
> +	l_dbus_object_remove_interface(dbus_get_bus(),
> +					wsc->ops->get_path(wsc->user_data),
> +					IWD_WSC_INTERFACE);
> +}
> +
> +void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
> +			struct scan_bss *target, const char *pin)
> +{
> +	wsc->netdev = netdev;
> +	wsc->target = target;
> +	wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
> +		WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
> +	wsc->pin = l_strdup(pin);
> +
> +	wsc_connect(wsc);
> +}
> +
> +void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb)
> +{
> +	int r;
> +
> +	if (!wsc->wsc_association)
> +		return;
> +
> +	r = netdev_disconnect(wsc->netdev, cb, wsc->user_data);
> +	if (r == 0)
> +		return;
> +
> +	l_warn("Unable to initiate disconnect: %s", strerror(-r));
> +	wsc_connect_cleanup(wsc);
> +	wsc->ops->enrollee_done_cb(-ECANCELED, NULL, 0, wsc->user_data);
> +
> +	if (cb)
> +		cb(wsc->netdev, false, wsc->user_data);
> +}
> +
> +static const char *wsc_device_get_path(void *user_data)
> +{
> +	struct wsc *wsc = user_data;
> +
> +	if (!wsc || !wsc->netdev)
> +		return NULL;
> +
> +	return netdev_get_path(wsc->netdev);
> +}
> +
> +static void wsc_device_connect(struct l_dbus_message *message, const char *pin,
> +				void *user_data)
> +{
> +	struct wsc *wsc = user_data;
> +	struct l_dbus *dbus = dbus_get_bus();
> +	unsigned int walk_time;
> +	enum wsc_device_password_id dpid;
> +	scan_notify_func_t results_cb;
> +
> +	if (wsc->pending) {
> +		l_dbus_send(dbus, dbus_error_busy(message));
> +		return;
> +	}
> +
> +	wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
> +	if (!wsc->station) {
> +		l_dbus_send(dbus, dbus_error_not_available(message));
> +		return;
> +	}
> +
> +	if (pin) {
> +		if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
> +			dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
> +		else
> +			dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
> +
> +		walk_time = 60;
> +		results_cb = pin_scan_results;
> +	} else {
> +		dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
> +		walk_time = WALK_TIME;
> +		results_cb = push_button_scan_results;
> +	}
> +
> +	if (!wsc_initiate_scan(wsc, dpid, results_cb)) {
> +		l_dbus_send(dbus, dbus_error_failed(message));
> +		return;
> +	}
> +
> +	wsc->walk_timer = l_timeout_create(walk_time, walk_timeout, wsc, NULL);
> +	wsc->pending = l_dbus_message_ref(message);
> +}
> +
> +static void wsc_device_cancel(struct l_dbus_message *message, void *user_data)
> +{
> +	struct wsc *wsc = user_data;
> +	struct l_dbus *dbus = dbus_get_bus();
> +
> +	if (!wsc->pending) {
> +		l_dbus_send(dbus, dbus_error_not_available(message));
> +		return;
> +	}
> +
> +	if (wsc->pending_cancel) {
> +		l_dbus_send(dbus, dbus_error_busy(message));
> +		return;
> +	}
> +

As mentioned before, this doesn't seem like it belongs in the op itself 
but in the higher layer...

> +	wsc_cancel_scan(wsc);
> +
> +	if (wsc->station_state_watch) {
> +		station_remove_state_watch(wsc->station,
> +						wsc->station_state_watch);
> +		wsc->station_state_watch = 0;
> +		wsc->target = NULL;
> +	}
> +
> +	if (wsc->wsc_association) {
> +		wsc->pending_cancel = l_dbus_message_ref(message);
> +		wsc_cancel(wsc, wsc_disconnect_cb);
> +		return;
> +	}
> +
> +	dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
> +	l_dbus_send(dbus, l_dbus_message_new_method_return(message));

As is this...

> +}
> +
> +static void wsc_device_enrollee_done(int err, struct wsc_credentials_info *creds,
> +					unsigned int n_creds, void *user_data)
> +{
> +	struct wsc *wsc = user_data;
> +
> +	l_debug("err=%i", err);
> +
> +	switch (err) {
> +	case 0:
> +		break;
> +	case -ECANCELED:
> +		dbus_pending_reply(&wsc->pending,
> +					dbus_error_aborted(wsc->pending));
> +		return;
> +	case -ENOKEY:
> +		dbus_pending_reply(&wsc->pending,
> +					wsc_error_no_credentials(wsc->pending));
> +		return;
> +	case -EBUSY:
> +		dbus_pending_reply(&wsc->pending,
> +					dbus_error_busy(wsc->pending));
> +		return;
> +	default:
> +		dbus_pending_reply(&wsc->pending,
> +					dbus_error_failed(wsc->pending));
> +		return;
> +	}
> +
> +	wsc_store_credentials(creds, n_creds);
> +	wsc_try_credentials(wsc, creds, n_creds);
> +}
> +
> +static const struct wsc_ops wsc_device_ops = {
> +	.get_path = wsc_device_get_path,
> +	.connect = wsc_device_connect,
> +	.cancel = wsc_device_cancel,
> +	.enrollee_done_cb = wsc_device_enrollee_done,
> +};
> +
> +static void wsc_device_new(struct netdev *netdev)
>   {
>   	struct l_dbus *dbus = dbus_get_bus();
>   	struct wsc *wsc;
> @@ -1167,15 +1247,17 @@ static void wsc_add_interface(struct netdev *netdev)
>   		return;
>   	}
>   
> -	wsc = l_new(struct wsc, 1);
> +	wsc = wsc_new(&wsc_device_ops, NULL);
> +	wsc->user_data = wsc;
>   	wsc->netdev = netdev;
>   
> -	if (!l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
> -						IWD_WSC_INTERFACE,
> -						wsc)) {
> -		wsc_free(wsc);
> -		l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
> -	}
> +	if (l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
> +					IWD_WSC_INTERFACE, wsc))
> +		return;
> +
> +	wsc_free(wsc);
> +	l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
> +		netdev_get_path(netdev));

This logic flipping seems gratuitous, I'd just leave it the way it was...

>   }
>   
>   static void wsc_remove_interface(struct netdev *netdev)
> @@ -1194,7 +1276,7 @@ static void wsc_netdev_watch(struct netdev *netdev,
>   	case NETDEV_WATCH_EVENT_NEW:
>   		if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION &&
>   				netdev_get_is_up(netdev))
> -			wsc_add_interface(netdev);
> +			wsc_device_new(netdev);
>   		break;
>   	case NETDEV_WATCH_EVENT_DOWN:
>   	case NETDEV_WATCH_EVENT_DEL:
> diff --git a/src/wsc.h b/src/wsc.h
> index ada08c89..e8a965f2 100644
> --- a/src/wsc.h
> +++ b/src/wsc.h
> @@ -20,6 +20,11 @@
>    *
>    */
>   
> +struct wsc;
> +struct netdev;
> +struct scan_bss;
> +struct iovec;
> +
>   struct wsc_credentials_info {
>   	char ssid[33];
>   	enum security security;
> @@ -31,5 +36,17 @@ struct wsc_credentials_info {
>   	bool has_passphrase;
>   };
>   
> -typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
> -				unsigned int n_creds, void *user_data);
> +struct wsc_ops {
> +	const char *(*get_path)(void *user_data);
> +	void (*connect)(struct l_dbus_message *message, const char *pin,
> +			void *user_data);
> +	void (*cancel)(struct l_dbus_message *message, void *user_data);
> +	void (*enrollee_done_cb)(int err, struct wsc_credentials_info *creds,
> +					unsigned int n_creds, void *user_data);

So one thing I find very weird about this setup is the fact that you're 
mixing operations with a callback in something called 'wsc_ops'.  Can't 
your 'connect' operation simply set the callback or perhaps provide the 
callback directly to wsc_start_enrollee?

> +};
> +
> +struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data);
> +void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
> +			struct scan_bss *target, const char *pin);
> +void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb);
> +void wsc_destroy(struct wsc *wsc);
> 

Regards,
-Denis

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Andrew Zaborowski]

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

On Mon, 6 Jan 2020 at 23:00, Denis Kenzior <denkenz@gmail.com> wrote:
> On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
> > @@ -210,8 +210,6 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
> >
> >       l_debug("%p, success: %d", wsc, success);
> >
> > -     wsc->wsc_association = false;
> > -
> >       reply = l_dbus_message_new_method_return(wsc->pending_cancel);
> >       l_dbus_message_set_arguments(reply, "");
> >       dbus_pending_reply(&wsc->pending_cancel, reply);
>
> Is this chunk a straggler from some earlier refactoring?  If so, then a
> separate patch would be desirable

So in the new setup we have parts of wsc.c that are common between the
Station mode use case and the external user use case (i.e. P2P), and
then we have parts that are relevant to Station mode only.  We sort of
have 3 parts:

1. the core part that sets up the EAP method config, starts the
connection and receives the credentials.
2. the DBus interface that lives even when we're not doing WSC (also
common to both modes now)
3. the Station-specific code which interfaces between 1. and 2. and
does the scanning etc., i.e. everything used by @wsc_device_ops

wsc_disconnect_cb belongs in 3., while wsc->wsc_association belongs in
2. so wsc_disconnect_cb shouldn't be touching wsc->wsc_association.
It's being set to false in wsc_connect_cleanup which now gets called
from wsc_cancel before wsc_disconnect_cb.  I think that the
netdev_disconnect call also triggers an immediate wsc_connect_cb
callback which also calls wsc_connect_cleanup, so wsc->wsc_association
would have been always false.

>
> <snip>
>
> > @@ -525,40 +523,19 @@ static void wsc_connect(struct wsc *wsc)
> >   error:
> >       handshake_state_free(hs);
> >       wsc_connect_cleanup(wsc);
> > -     wsc->done_cb(r, NULL, 0, wsc->done_data);
> > +     wsc->ops->enrollee_done_cb(r, NULL, 0, wsc->user_data);
> >   }
> >
> > -/* Done callback for when WSC is triggered by DBus methods */
> > -static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
> > -                             unsigned int n_creds, void *user_data)
> > +static void wsc_connect_set_method(struct wsc *wsc)
> >   {
> > -     struct wsc *wsc = user_data;
> > -
> > -     l_debug("err=%i", err);
> > +     const char *pin;
> >
> > -     switch (err) {
> > -     case 0:
> > -             break;
> > -     case -ECANCELED:
> > -             dbus_pending_reply(&wsc->pending,
> > -                                     dbus_error_aborted(wsc->pending));
> > -             return;
> > -     case -ENOKEY:
> > -             dbus_pending_reply(&wsc->pending,
> > -                                     wsc_error_no_credentials(wsc->pending));
> > -             return;
> > -     case -EBUSY:
> > -             dbus_pending_reply(&wsc->pending,
> > -                                     dbus_error_busy(wsc->pending));
> > -             return;
> > -     default:
> > -             dbus_pending_reply(&wsc->pending,
> > -                                     dbus_error_failed(wsc->pending));
> > -             return;
> > -     }
> > +     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
> > +             l_dbus_message_get_arguments(wsc->pending, "s", &pin);
> > +     else
> > +             pin = NULL;
> >
> > -     wsc_store_credentials(wsc);
> > -     wsc_try_credentials(wsc);
> > +     wsc_start_enrollee(wsc, wsc->netdev, wsc->target, pin, NULL, 0);
>
> Is wsc_start_enrollee going to be called from p2p directly or only via
> the D-Bus API?

So the D-Bus API will call the P2P code (to do GO Negotiation /
Provision Discovery etc.) or the station-specific code inside wsc.c
(to do the scanning) and they'll both call wsc_start_enrollee when
ready.  wsc_connect_set_method is the station-specific code path where
this happens.

> This enclosing function (wsc_connect_set_method) is now
> quite strange looking.  Maybe it'd look nicer to just set wsc->pin from
> inside the StartPin / PushButton handlers instead and avoid this completely?

Well I can do this.  I preferred to keep the parts 1. and 2. of the
code independent (e.g. for scenarios like auto-connecting to some P2P
devices in the future) but it makes little differnce.

I notice the way git formatted this patch makes it hard to read.

>
> >   }
> >
> >   static void station_state_watch(enum station_state state, void *userdata)
> > @@ -573,7 +550,7 @@ static void station_state_watch(enum station_state state, void *userdata)
> >       station_remove_state_watch(wsc->station, wsc->station_state_watch);
> >       wsc->station_state_watch = 0;
> >
> > -     wsc_connect(wsc);
> > +     wsc_connect_set_method(wsc);
> >   }
> >
> >   static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
> > @@ -587,25 +564,9 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
> >       wsc->target = target;
> >       station_set_autoconnect(wsc->station, false);
> >
> > -     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
> > -             char *pin;
> > -
> > -             wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
> > -
> > -             if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
> > -                     goto error;
> > -
> > -             wsc->pin = l_strdup(pin);
> > -     } else
> > -             wsc->config_method =
> > -                     WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
> > -
> > -     wsc->done_cb = wsc_dbus_done_cb;
> > -     wsc->done_data = wsc;
> > -
> >       switch (station_get_state(wsc->station)) {
> >       case STATION_STATE_DISCONNECTED:
> > -             wsc_connect(wsc);
> > +             wsc_connect_set_method(wsc);
> >               return;
> >       case STATION_STATE_CONNECTING:
> >       case STATION_STATE_CONNECTED:
> > @@ -652,20 +613,13 @@ static void walk_timeout(struct l_timeout *timeout, void *user_data)
> >
> >       wsc_cancel_scan(wsc);
> >
> > -     if (wsc->pending)
> > -             dbus_pending_reply(&wsc->pending,
> > -                             wsc_error_walk_time_expired(wsc->pending));
> > -}
> > +     if (wsc->pending) {
> > +             struct l_dbus_message *reply = wsc->pin ?
> > +                     wsc_error_time_expired(wsc->pending) :
> > +                     wsc_error_walk_time_expired(wsc->pending);
> >
> > -static void pin_timeout(struct l_timeout *timeout, void *user_data)
> > -{
> > -     struct wsc *wsc = user_data;
> > -
> > -     wsc_cancel_scan(wsc);
> > -
> > -     if (wsc->pending)
> > -             dbus_pending_reply(&wsc->pending,
> > -                                     wsc_error_time_expired(wsc->pending));
> > +             dbus_pending_reply(&wsc->pending, reply);
> > +     }
> >   }
> >
>
> Okay, but this really belongs as a separate patch

Ok.

>
> >   static bool push_button_scan_results(int err, struct l_queue *bss_list,
> > @@ -1001,20 +955,7 @@ static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
> >
> >       l_debug("");
> >
> > -     if (wsc->pending)
> > -             return dbus_error_busy(message);
> > -
>
> So why would you move this check out into the ops provided operation and
> not keep it here?
>
...
> > -     wsc->pending = l_dbus_message_ref(message);
> > -
>
> Pending tracking should also likely be managed here...
>
...
> > -     if (wsc->pending)
> > -             return dbus_error_busy(message);
>
> Same comments as above apply here ...

For one, in the P2P case, it's not going to have access to the
internals of struct wsc, so if P2P replies with success or an error
message to the PushButton call, it would need some way to also clear
wsc->pending.  That's doable but it seems simpler to leave full
control to the actual implementation.  So my idea in this version was
that the DBus handlers would be really simple and stateless.

Also remember that in the P2P case the PushButton method will kick off
the connection process but there's a few different steps and WSC is
just one of them and might be skipped completely in some scenarios.

>
> > -
> > -     wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
> > -     if (!wsc->station)
> > -             return dbus_error_not_available(message);
> > -
> >       if (!l_dbus_message_get_arguments(message, "s", &pin))
> >               return dbus_error_invalid_args(message);
> >
> >       if (!wsc_pin_is_valid(pin))
> >               return dbus_error_invalid_format(message);
> >
> > -     if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
> > -             dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
> > -     else
> > -             dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
> > -
> > -     if (!wsc_initiate_scan(wsc, dpid, pin_scan_results))
> > -             return dbus_error_failed(message);
> > -
> > -     wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
> > -     wsc->pending = l_dbus_message_ref(message);
> > -
> > +     wsc->ops->connect(message, pin, wsc->user_data);
> >       return NULL;
> >   }
> >
> > -static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
> > +static struct l_dbus_message *wsc_dbus_cancel(struct l_dbus *dbus,
> >                                               struct l_dbus_message *message,
> >                                               void *user_data)
> >   {
> >       struct wsc *wsc = user_data;
> > -     struct l_dbus_message *reply;
> >
> >       l_debug("");
> >
> > -     if (!wsc->pending)
> > -             return dbus_error_not_available(message);
> > -
> > -     wsc_cancel_scan(wsc);
> > -
> > -     if (wsc->station_state_watch) {
> > -             station_remove_state_watch(wsc->station,
> > -                                             wsc->station_state_watch);
> > -             wsc->station_state_watch = 0;
> > -             wsc->target = NULL;
> > -     }
> > -
> > -     if (wsc->wsc_association) {
> > -             int r;
> > -
> > -             r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
> > -             if (r == 0) {
> > -                     wsc->pending_cancel = l_dbus_message_ref(message);
> > -                     return NULL;
> > -             }
> > -
> > -             l_warn("Unable to initiate disconnect: %s", strerror(-r));
> > -             wsc->wsc_association = false;
> > -     }
> > -
> > -     dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
> > -
> > -     reply = l_dbus_message_new_method_return(message);
> > -     l_dbus_message_set_arguments(reply, "");
> > -
> > -     return reply;
> > +     wsc->ops->cancel(message, wsc->user_data);
> > +     return NULL;
> >   }
> >
> >   static void setup_wsc_interface(struct l_dbus_interface *interface)
> > @@ -1128,7 +1021,7 @@ static void setup_wsc_interface(struct l_dbus_interface *interface)
> >       l_dbus_interface_method(interface, "StartPin", 0,
> >                               wsc_start_pin, "", "s", "pin");
> >       l_dbus_interface_method(interface, "Cancel", 0,
> > -                             wsc_cancel, "", "");
> > +                             wsc_dbus_cancel, "", "");
> >   }
> >
> >   static void wsc_free(void *userdata)
> > @@ -1155,7 +1048,194 @@ static void wsc_free(void *userdata)
> >       l_free(wsc);
> >   }
> >
> > -static void wsc_add_interface(struct netdev *netdev)
> > +struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data)
> > +{
> > +     struct l_dbus *dbus = dbus_get_bus();
> > +     struct wsc *wsc;
> > +     const char *path;
> > +
> > +     wsc = l_new(struct wsc, 1);
> > +     wsc->ops = ops;
> > +     wsc->user_data = user_data;
>
> This part seems rather weird

Do you mean the whole function? and why?

>
> > +
> > +     path = wsc->ops->get_path(wsc->user_data);
> > +     if (!path)
> > +             return wsc;
> > +
> > +     if (l_dbus_object_add_interface(dbus, path, IWD_WSC_INTERFACE, wsc))
> > +             return wsc;
> > +
> > +     wsc_free(wsc);
> > +     l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
> > +             path);
> > +     return NULL;
> > +}
> > +
> > +void wsc_destroy(struct wsc *wsc)
> > +{
> > +     l_dbus_object_remove_interface(dbus_get_bus(),
> > +                                     wsc->ops->get_path(wsc->user_data),
> > +                                     IWD_WSC_INTERFACE);
> > +}
> > +
> > +void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
> > +                     struct scan_bss *target, const char *pin)
> > +{
> > +     wsc->netdev = netdev;
> > +     wsc->target = target;
> > +     wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
> > +             WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
> > +     wsc->pin = l_strdup(pin);
> > +
> > +     wsc_connect(wsc);
> > +}
> > +
> > +void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb)
> > +{
> > +     int r;
> > +
> > +     if (!wsc->wsc_association)
> > +             return;
> > +
> > +     r = netdev_disconnect(wsc->netdev, cb, wsc->user_data);
> > +     if (r == 0)
> > +             return;
> > +
> > +     l_warn("Unable to initiate disconnect: %s", strerror(-r));
> > +     wsc_connect_cleanup(wsc);
> > +     wsc->ops->enrollee_done_cb(-ECANCELED, NULL, 0, wsc->user_data);
> > +
> > +     if (cb)
> > +             cb(wsc->netdev, false, wsc->user_data);
> > +}
> > +
> > +static const char *wsc_device_get_path(void *user_data)
> > +{
> > +     struct wsc *wsc = user_data;
> > +
> > +     if (!wsc || !wsc->netdev)
> > +             return NULL;
> > +
> > +     return netdev_get_path(wsc->netdev);
> > +}
> > +
> > +static void wsc_device_connect(struct l_dbus_message *message, const char *pin,
> > +                             void *user_data)
> > +{
> > +     struct wsc *wsc = user_data;
> > +     struct l_dbus *dbus = dbus_get_bus();
> > +     unsigned int walk_time;
> > +     enum wsc_device_password_id dpid;
> > +     scan_notify_func_t results_cb;
> > +
> > +     if (wsc->pending) {
> > +             l_dbus_send(dbus, dbus_error_busy(message));
> > +             return;
> > +     }
> > +
> > +     wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
> > +     if (!wsc->station) {
> > +             l_dbus_send(dbus, dbus_error_not_available(message));
> > +             return;
> > +     }
> > +
> > +     if (pin) {
> > +             if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
> > +                     dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
> > +             else
> > +                     dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
> > +
> > +             walk_time = 60;
> > +             results_cb = pin_scan_results;
> > +     } else {
> > +             dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
> > +             walk_time = WALK_TIME;
> > +             results_cb = push_button_scan_results;
> > +     }
> > +
> > +     if (!wsc_initiate_scan(wsc, dpid, results_cb)) {
> > +             l_dbus_send(dbus, dbus_error_failed(message));
> > +             return;
> > +     }
> > +
> > +     wsc->walk_timer = l_timeout_create(walk_time, walk_timeout, wsc, NULL);
> > +     wsc->pending = l_dbus_message_ref(message);
> > +}
> > +
> > +static void wsc_device_cancel(struct l_dbus_message *message, void *user_data)
> > +{
> > +     struct wsc *wsc = user_data;
> > +     struct l_dbus *dbus = dbus_get_bus();
> > +
> > +     if (!wsc->pending) {
> > +             l_dbus_send(dbus, dbus_error_not_available(message));
> > +             return;
> > +     }
> > +
> > +     if (wsc->pending_cancel) {
> > +             l_dbus_send(dbus, dbus_error_busy(message));
> > +             return;
> > +     }
> > +
>
> As mentioned before, this doesn't seem like it belongs in the op itself
> but in the higher layer...
>
> > +     wsc_cancel_scan(wsc);
> > +
> > +     if (wsc->station_state_watch) {
> > +             station_remove_state_watch(wsc->station,
> > +                                             wsc->station_state_watch);
> > +             wsc->station_state_watch = 0;
> > +             wsc->target = NULL;
> > +     }
> > +
> > +     if (wsc->wsc_association) {
> > +             wsc->pending_cancel = l_dbus_message_ref(message);
> > +             wsc_cancel(wsc, wsc_disconnect_cb);
> > +             return;
> > +     }
> > +
> > +     dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
> > +     l_dbus_send(dbus, l_dbus_message_new_method_return(message));
>
> As is this...
>
> > +}
> > +
> > +static void wsc_device_enrollee_done(int err, struct wsc_credentials_info *creds,
> > +                                     unsigned int n_creds, void *user_data)
> > +{
> > +     struct wsc *wsc = user_data;
> > +
> > +     l_debug("err=%i", err);
> > +
> > +     switch (err) {
> > +     case 0:
> > +             break;
> > +     case -ECANCELED:
> > +             dbus_pending_reply(&wsc->pending,
> > +                                     dbus_error_aborted(wsc->pending));
> > +             return;
> > +     case -ENOKEY:
> > +             dbus_pending_reply(&wsc->pending,
> > +                                     wsc_error_no_credentials(wsc->pending));
> > +             return;
> > +     case -EBUSY:
> > +             dbus_pending_reply(&wsc->pending,
> > +                                     dbus_error_busy(wsc->pending));
> > +             return;
> > +     default:
> > +             dbus_pending_reply(&wsc->pending,
> > +                                     dbus_error_failed(wsc->pending));
> > +             return;
> > +     }
> > +
> > +     wsc_store_credentials(creds, n_creds);
> > +     wsc_try_credentials(wsc, creds, n_creds);
> > +}
> > +
> > +static const struct wsc_ops wsc_device_ops = {
> > +     .get_path = wsc_device_get_path,
> > +     .connect = wsc_device_connect,
> > +     .cancel = wsc_device_cancel,
> > +     .enrollee_done_cb = wsc_device_enrollee_done,
> > +};
> > +
> > +static void wsc_device_new(struct netdev *netdev)
> >   {
> >       struct l_dbus *dbus = dbus_get_bus();
> >       struct wsc *wsc;
> > @@ -1167,15 +1247,17 @@ static void wsc_add_interface(struct netdev *netdev)
> >               return;
> >       }
> >
> > -     wsc = l_new(struct wsc, 1);
> > +     wsc = wsc_new(&wsc_device_ops, NULL);
> > +     wsc->user_data = wsc;
> >       wsc->netdev = netdev;
> >
> > -     if (!l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
> > -                                             IWD_WSC_INTERFACE,
> > -                                             wsc)) {
> > -             wsc_free(wsc);
> > -             l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
> > -     }
> > +     if (l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
> > +                                     IWD_WSC_INTERFACE, wsc))
> > +             return;
> > +
> > +     wsc_free(wsc);
> > +     l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
> > +             netdev_get_path(netdev));
>
> This logic flipping seems gratuitous, I'd just leave it the way it was...

Ok.

>
> >   }
> >
> >   static void wsc_remove_interface(struct netdev *netdev)
> > @@ -1194,7 +1276,7 @@ static void wsc_netdev_watch(struct netdev *netdev,
> >       case NETDEV_WATCH_EVENT_NEW:
> >               if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION &&
> >                               netdev_get_is_up(netdev))
> > -                     wsc_add_interface(netdev);
> > +                     wsc_device_new(netdev);
> >               break;
> >       case NETDEV_WATCH_EVENT_DOWN:
> >       case NETDEV_WATCH_EVENT_DEL:
> > diff --git a/src/wsc.h b/src/wsc.h
> > index ada08c89..e8a965f2 100644
> > --- a/src/wsc.h
> > +++ b/src/wsc.h
> > @@ -20,6 +20,11 @@
> >    *
> >    */
> >
> > +struct wsc;
> > +struct netdev;
> > +struct scan_bss;
> > +struct iovec;
> > +
> >   struct wsc_credentials_info {
> >       char ssid[33];
> >       enum security security;
> > @@ -31,5 +36,17 @@ struct wsc_credentials_info {
> >       bool has_passphrase;
> >   };
> >
> > -typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
> > -                             unsigned int n_creds, void *user_data);
> > +struct wsc_ops {
> > +     const char *(*get_path)(void *user_data);
> > +     void (*connect)(struct l_dbus_message *message, const char *pin,
> > +                     void *user_data);
> > +     void (*cancel)(struct l_dbus_message *message, void *user_data);
> > +     void (*enrollee_done_cb)(int err, struct wsc_credentials_info *creds,
> > +                                     unsigned int n_creds, void *user_data);
>
> So one thing I find very weird about this setup is the fact that you're
> mixing operations with a callback in something called 'wsc_ops'.  Can't
> your 'connect' operation simply set the callback or perhaps provide the
> callback directly to wsc_start_enrollee?

It would work equally well, yes.  Maybe I was being lazy, I'll fix this.

One argument for keeping all the callbacks in this struct is that they
implicitly use the same "user_data" and so we save both the callback
parameter to wsc_start_enrollee, the user_data parameter (we often
complained about our functions having too many arguments) and 2 fields
in struct wsc for storing these two parameters.

Best regards

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Denis Kenzior]

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

Hi Andrew,

On 1/6/20 8:55 PM, Andrew Zaborowski wrote:
> On Mon, 6 Jan 2020 at 23:00, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
>>> @@ -210,8 +210,6 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
>>>
>>>        l_debug("%p, success: %d", wsc, success);
>>>
>>> -     wsc->wsc_association = false;
>>> -
>>>        reply = l_dbus_message_new_method_return(wsc->pending_cancel);
>>>        l_dbus_message_set_arguments(reply, "");
>>>        dbus_pending_reply(&wsc->pending_cancel, reply);
>>
>> Is this chunk a straggler from some earlier refactoring?  If so, then a
>> separate patch would be desirable
> 
> So in the new setup we have parts of wsc.c that are common between the
> Station mode use case and the external user use case (i.e. P2P), and
> then we have parts that are relevant to Station mode only.  We sort of
> have 3 parts:
> 
> 1. the core part that sets up the EAP method config, starts the
> connection and receives the credentials.
> 2. the DBus interface that lives even when we're not doing WSC (also
> common to both modes now)
> 3. the Station-specific code which interfaces between 1. and 2. and
> does the scanning etc., i.e. everything used by @wsc_device_ops
> 
> wsc_disconnect_cb belongs in 3., while wsc->wsc_association belongs in
> 2. so wsc_disconnect_cb shouldn't be touching wsc->wsc_association.

Not quite sure I follow.  wsc_disconnect_cb is called when cancel is 
invoked via D-Bus API (or in your proposal it would be invoked via 
wsc_cancel).  wsc->association tracks whether we have a connection 
established for the purposes of EAP-WSC.  Hence if wsc->association is 
true, we have to disconnect it first.  I see these two as inherently linked?

> It's being set to false in wsc_connect_cleanup which now gets called
> from wsc_cancel before wsc_disconnect_cb.  I think that the
> netdev_disconnect call also triggers an immediate wsc_connect_cb
> callback which also calls wsc_connect_cleanup, so wsc->wsc_association
> would have been always false.

Right.  The intent was for connect callback to be called with aborted or 
whatever and have that clean up the state.  You end up calling 
wsc_connect_cleanup multiple times.  This is probably okay, but seems 
unnecessary.

> 
>>
>> <snip>
>>
>>> @@ -525,40 +523,19 @@ static void wsc_connect(struct wsc *wsc)
>>>    error:
>>>        handshake_state_free(hs);
>>>        wsc_connect_cleanup(wsc);
>>> -     wsc->done_cb(r, NULL, 0, wsc->done_data);
>>> +     wsc->ops->enrollee_done_cb(r, NULL, 0, wsc->user_data);
>>>    }
>>>
>>> -/* Done callback for when WSC is triggered by DBus methods */
>>> -static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
>>> -                             unsigned int n_creds, void *user_data)
>>> +static void wsc_connect_set_method(struct wsc *wsc)
>>>    {
>>> -     struct wsc *wsc = user_data;
>>> -
>>> -     l_debug("err=%i", err);
>>> +     const char *pin;
>>>
>>> -     switch (err) {
>>> -     case 0:
>>> -             break;
>>> -     case -ECANCELED:
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                                     dbus_error_aborted(wsc->pending));
>>> -             return;
>>> -     case -ENOKEY:
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                                     wsc_error_no_credentials(wsc->pending));
>>> -             return;
>>> -     case -EBUSY:
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                                     dbus_error_busy(wsc->pending));
>>> -             return;
>>> -     default:
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                                     dbus_error_failed(wsc->pending));
>>> -             return;
>>> -     }
>>> +     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
>>> +             l_dbus_message_get_arguments(wsc->pending, "s", &pin);
>>> +     else
>>> +             pin = NULL;
>>>
>>> -     wsc_store_credentials(wsc);
>>> -     wsc_try_credentials(wsc);
>>> +     wsc_start_enrollee(wsc, wsc->netdev, wsc->target, pin, NULL, 0);
>>
>> Is wsc_start_enrollee going to be called from p2p directly or only via
>> the D-Bus API?
> 
> So the D-Bus API will call the P2P code (to do GO Negotiation /
> Provision Discovery etc.) or the station-specific code inside wsc.c
> (to do the scanning) and they'll both call wsc_start_enrollee when
> ready.  wsc_connect_set_method is the station-specific code path where
> this happens.

Okay, but the PIN / PushButton setup is common to both, no?  And it 
seems that would logically belong in the overall D-Bus layer handlers. 
No point passing pin around, especially since you already made the 
change to store it inside the wsc object.

> 
>> This enclosing function (wsc_connect_set_method) is now
>> quite strange looking.  Maybe it'd look nicer to just set wsc->pin from
>> inside the StartPin / PushButton handlers instead and avoid this completely?
> 
> Well I can do this.  I preferred to keep the parts 1. and 2. of the
> code independent (e.g. for scenarios like auto-connecting to some P2P
> devices in the future) but it makes little differnce.
> 
> I notice the way git formatted this patch makes it hard to read.
> 
>>
>>>    }
>>>
>>>    static void station_state_watch(enum station_state state, void *userdata)
>>> @@ -573,7 +550,7 @@ static void station_state_watch(enum station_state state, void *userdata)
>>>        station_remove_state_watch(wsc->station, wsc->station_state_watch);
>>>        wsc->station_state_watch = 0;
>>>
>>> -     wsc_connect(wsc);
>>> +     wsc_connect_set_method(wsc);
>>>    }
>>>
>>>    static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
>>> @@ -587,25 +564,9 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
>>>        wsc->target = target;
>>>        station_set_autoconnect(wsc->station, false);
>>>
>>> -     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
>>> -             char *pin;
>>> -
>>> -             wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
>>> -
>>> -             if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
>>> -                     goto error;
>>> -
>>> -             wsc->pin = l_strdup(pin);
>>> -     } else
>>> -             wsc->config_method =
>>> -                     WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
>>> -
>>> -     wsc->done_cb = wsc_dbus_done_cb;
>>> -     wsc->done_data = wsc;
>>> -
>>>        switch (station_get_state(wsc->station)) {
>>>        case STATION_STATE_DISCONNECTED:
>>> -             wsc_connect(wsc);
>>> +             wsc_connect_set_method(wsc);
>>>                return;
>>>        case STATION_STATE_CONNECTING:
>>>        case STATION_STATE_CONNECTED:
>>> @@ -652,20 +613,13 @@ static void walk_timeout(struct l_timeout *timeout, void *user_data)
>>>
>>>        wsc_cancel_scan(wsc);
>>>
>>> -     if (wsc->pending)
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                             wsc_error_walk_time_expired(wsc->pending));
>>> -}
>>> +     if (wsc->pending) {
>>> +             struct l_dbus_message *reply = wsc->pin ?
>>> +                     wsc_error_time_expired(wsc->pending) :
>>> +                     wsc_error_walk_time_expired(wsc->pending);
>>>
>>> -static void pin_timeout(struct l_timeout *timeout, void *user_data)
>>> -{
>>> -     struct wsc *wsc = user_data;
>>> -
>>> -     wsc_cancel_scan(wsc);
>>> -
>>> -     if (wsc->pending)
>>> -             dbus_pending_reply(&wsc->pending,
>>> -                                     wsc_error_time_expired(wsc->pending));
>>> +             dbus_pending_reply(&wsc->pending, reply);
>>> +     }
>>>    }
>>>
>>
>> Okay, but this really belongs as a separate patch
> 
> Ok.
> 
>>
>>>    static bool push_button_scan_results(int err, struct l_queue *bss_list,
>>> @@ -1001,20 +955,7 @@ static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
>>>
>>>        l_debug("");
>>>
>>> -     if (wsc->pending)
>>> -             return dbus_error_busy(message);
>>> -
>>
>> So why would you move this check out into the ops provided operation and
>> not keep it here?
>>
> ...
>>> -     wsc->pending = l_dbus_message_ref(message);
>>> -
>>
>> Pending tracking should also likely be managed here...
>>
> ...
>>> -     if (wsc->pending)
>>> -             return dbus_error_busy(message);
>>
>> Same comments as above apply here ...
> 
> For one, in the P2P case, it's not going to have access to the
> internals of struct wsc, so if P2P replies with success or an error
> message to the PushButton call, it would need some way to also clear
> wsc->pending.  That's doable but it seems simpler to leave full
> control to the actual implementation.  So my idea in this version was
> that the DBus handlers would be really simple and stateless.
> 

But how would p2p set wsc->pending if it has no access to the wsc 
object?  Or are you duplicating the D-Bus handling code inside p2p?  I 
guess I was under the impression that you'd want to unify the D-Bus bits 
as well.

If you want to re-invent this inside p2p, then there's really no need 
for this wsc_ops abstraction.  Just have something like:

struct wsc_dbus_object {
	bool station;
	union {
		struct wsc_p2p *p2p;
		struct wsc *wsc;
	}
}

> Also remember that in the P2P case the PushButton method will kick off
> the connection process but there's a few different steps and WSC is
> just one of them and might be skipped completely in some scenarios.
> 

Sure, so you'd abstract wsc eap connection behind 
wsc_eap_start_enrollee, wsc_eap_cancel, etc.  This ends up being re-used 
and the rest you can do separate.

>>
>>> -
>>> -     wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
>>> -     if (!wsc->station)
>>> -             return dbus_error_not_available(message);
>>> -
>>>        if (!l_dbus_message_get_arguments(message, "s", &pin))
>>>                return dbus_error_invalid_args(message);
>>>
>>>        if (!wsc_pin_is_valid(pin))
>>>                return dbus_error_invalid_format(message);
>>>
>>> -     if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
>>> -             dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
>>> -     else
>>> -             dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
>>> -
>>> -     if (!wsc_initiate_scan(wsc, dpid, pin_scan_results))
>>> -             return dbus_error_failed(message);
>>> -
>>> -     wsc->walk_timer = l_timeout_create(60, pin_timeout, wsc, NULL);
>>> -     wsc->pending = l_dbus_message_ref(message);
>>> -
>>> +     wsc->ops->connect(message, pin, wsc->user_data);
>>>        return NULL;
>>>    }
>>>
>>> -static struct l_dbus_message *wsc_cancel(struct l_dbus *dbus,
>>> +static struct l_dbus_message *wsc_dbus_cancel(struct l_dbus *dbus,
>>>                                                struct l_dbus_message *message,
>>>                                                void *user_data)
>>>    {
>>>        struct wsc *wsc = user_data;
>>> -     struct l_dbus_message *reply;
>>>
>>>        l_debug("");
>>>
>>> -     if (!wsc->pending)
>>> -             return dbus_error_not_available(message);
>>> -
>>> -     wsc_cancel_scan(wsc);
>>> -
>>> -     if (wsc->station_state_watch) {
>>> -             station_remove_state_watch(wsc->station,
>>> -                                             wsc->station_state_watch);
>>> -             wsc->station_state_watch = 0;
>>> -             wsc->target = NULL;
>>> -     }
>>> -
>>> -     if (wsc->wsc_association) {
>>> -             int r;
>>> -
>>> -             r = netdev_disconnect(wsc->netdev, wsc_disconnect_cb, wsc);
>>> -             if (r == 0) {
>>> -                     wsc->pending_cancel = l_dbus_message_ref(message);
>>> -                     return NULL;
>>> -             }
>>> -
>>> -             l_warn("Unable to initiate disconnect: %s", strerror(-r));
>>> -             wsc->wsc_association = false;
>>> -     }
>>> -
>>> -     dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
>>> -
>>> -     reply = l_dbus_message_new_method_return(message);
>>> -     l_dbus_message_set_arguments(reply, "");
>>> -
>>> -     return reply;
>>> +     wsc->ops->cancel(message, wsc->user_data);
>>> +     return NULL;
>>>    }
>>>
>>>    static void setup_wsc_interface(struct l_dbus_interface *interface)
>>> @@ -1128,7 +1021,7 @@ static void setup_wsc_interface(struct l_dbus_interface *interface)
>>>        l_dbus_interface_method(interface, "StartPin", 0,
>>>                                wsc_start_pin, "", "s", "pin");
>>>        l_dbus_interface_method(interface, "Cancel", 0,
>>> -                             wsc_cancel, "", "");
>>> +                             wsc_dbus_cancel, "", "");
>>>    }
>>>
>>>    static void wsc_free(void *userdata)
>>> @@ -1155,7 +1048,194 @@ static void wsc_free(void *userdata)
>>>        l_free(wsc);
>>>    }
>>>
>>> -static void wsc_add_interface(struct netdev *netdev)
>>> +struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data)
>>> +{
>>> +     struct l_dbus *dbus = dbus_get_bus();
>>> +     struct wsc *wsc;
>>> +     const char *path;
>>> +
>>> +     wsc = l_new(struct wsc, 1);
>>> +     wsc->ops = ops;
>>> +     wsc->user_data = user_data;
>>
>> This part seems rather weird
> 
> Do you mean the whole function? and why?

I really meant the user_data setting.  You pass in user_data to all the 
callbacks, but it is set to NULL in wsc_new.  I think the intent was for 
user_data to be the wsc object itself, but I don't see how that would 
even be accomplished in this setup.  Have you actually tested this patch 
standalone?

> 
>>
>>> +
>>> +     path = wsc->ops->get_path(wsc->user_data);
>>> +     if (!path)
>>> +             return wsc;
>>> +
>>> +     if (l_dbus_object_add_interface(dbus, path, IWD_WSC_INTERFACE, wsc))
>>> +             return wsc;
>>> +
>>> +     wsc_free(wsc);
>>> +     l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
>>> +             path);
>>> +     return NULL;
>>> +}
>>> +
>>> +void wsc_destroy(struct wsc *wsc)
>>> +{
>>> +     l_dbus_object_remove_interface(dbus_get_bus(),
>>> +                                     wsc->ops->get_path(wsc->user_data),
>>> +                                     IWD_WSC_INTERFACE);
>>> +}
>>> +
>>> +void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
>>> +                     struct scan_bss *target, const char *pin)
>>> +{
>>> +     wsc->netdev = netdev;
>>> +     wsc->target = target;
>>> +     wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
>>> +             WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
>>> +     wsc->pin = l_strdup(pin);
>>> +
>>> +     wsc_connect(wsc);
>>> +}
>>> +
>>> +void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb)
>>> +{
>>> +     int r;
>>> +
>>> +     if (!wsc->wsc_association)
>>> +             return;
>>> +
>>> +     r = netdev_disconnect(wsc->netdev, cb, wsc->user_data);
>>> +     if (r == 0)
>>> +             return;
>>> +
>>> +     l_warn("Unable to initiate disconnect: %s", strerror(-r));
>>> +     wsc_connect_cleanup(wsc);
>>> +     wsc->ops->enrollee_done_cb(-ECANCELED, NULL, 0, wsc->user_data);
>>> +
>>> +     if (cb)
>>> +             cb(wsc->netdev, false, wsc->user_data);
>>> +}
>>> +
>>> +static const char *wsc_device_get_path(void *user_data)
>>> +{
>>> +     struct wsc *wsc = user_data;
>>> +
>>> +     if (!wsc || !wsc->netdev)
>>> +             return NULL;
>>> +
>>> +     return netdev_get_path(wsc->netdev);
>>> +}
>>> +
>>> +static void wsc_device_connect(struct l_dbus_message *message, const char *pin,
>>> +                             void *user_data)
>>> +{
>>> +     struct wsc *wsc = user_data;
>>> +     struct l_dbus *dbus = dbus_get_bus();
>>> +     unsigned int walk_time;
>>> +     enum wsc_device_password_id dpid;
>>> +     scan_notify_func_t results_cb;
>>> +
>>> +     if (wsc->pending) {
>>> +             l_dbus_send(dbus, dbus_error_busy(message));
>>> +             return;
>>> +     }
>>> +
>>> +     wsc->station = station_find(netdev_get_ifindex(wsc->netdev));
>>> +     if (!wsc->station) {
>>> +             l_dbus_send(dbus, dbus_error_not_available(message));
>>> +             return;
>>> +     }
>>> +
>>> +     if (pin) {
>>> +             if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
>>> +                     dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
>>> +             else
>>> +                     dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
>>> +
>>> +             walk_time = 60;
>>> +             results_cb = pin_scan_results;
>>> +     } else {
>>> +             dpid = WSC_DEVICE_PASSWORD_ID_PUSH_BUTTON;
>>> +             walk_time = WALK_TIME;
>>> +             results_cb = push_button_scan_results;
>>> +     }
>>> +
>>> +     if (!wsc_initiate_scan(wsc, dpid, results_cb)) {
>>> +             l_dbus_send(dbus, dbus_error_failed(message));
>>> +             return;
>>> +     }
>>> +
>>> +     wsc->walk_timer = l_timeout_create(walk_time, walk_timeout, wsc, NULL);
>>> +     wsc->pending = l_dbus_message_ref(message);
>>> +}
>>> +
>>> +static void wsc_device_cancel(struct l_dbus_message *message, void *user_data)
>>> +{
>>> +     struct wsc *wsc = user_data;
>>> +     struct l_dbus *dbus = dbus_get_bus();
>>> +
>>> +     if (!wsc->pending) {
>>> +             l_dbus_send(dbus, dbus_error_not_available(message));
>>> +             return;
>>> +     }
>>> +
>>> +     if (wsc->pending_cancel) {
>>> +             l_dbus_send(dbus, dbus_error_busy(message));
>>> +             return;
>>> +     }
>>> +
>>
>> As mentioned before, this doesn't seem like it belongs in the op itself
>> but in the higher layer...
>>
>>> +     wsc_cancel_scan(wsc);
>>> +
>>> +     if (wsc->station_state_watch) {
>>> +             station_remove_state_watch(wsc->station,
>>> +                                             wsc->station_state_watch);
>>> +             wsc->station_state_watch = 0;
>>> +             wsc->target = NULL;
>>> +     }
>>> +
>>> +     if (wsc->wsc_association) {
>>> +             wsc->pending_cancel = l_dbus_message_ref(message);
>>> +             wsc_cancel(wsc, wsc_disconnect_cb);
>>> +             return;
>>> +     }
>>> +
>>> +     dbus_pending_reply(&wsc->pending, dbus_error_aborted(wsc->pending));
>>> +     l_dbus_send(dbus, l_dbus_message_new_method_return(message));
>>
>> As is this...
>>
>>> +}
>>> +
>>> +static void wsc_device_enrollee_done(int err, struct wsc_credentials_info *creds,
>>> +                                     unsigned int n_creds, void *user_data)
>>> +{
>>> +     struct wsc *wsc = user_data;
>>> +
>>> +     l_debug("err=%i", err);
>>> +
>>> +     switch (err) {
>>> +     case 0:
>>> +             break;
>>> +     case -ECANCELED:
>>> +             dbus_pending_reply(&wsc->pending,
>>> +                                     dbus_error_aborted(wsc->pending));
>>> +             return;
>>> +     case -ENOKEY:
>>> +             dbus_pending_reply(&wsc->pending,
>>> +                                     wsc_error_no_credentials(wsc->pending));
>>> +             return;
>>> +     case -EBUSY:
>>> +             dbus_pending_reply(&wsc->pending,
>>> +                                     dbus_error_busy(wsc->pending));
>>> +             return;
>>> +     default:
>>> +             dbus_pending_reply(&wsc->pending,
>>> +                                     dbus_error_failed(wsc->pending));
>>> +             return;
>>> +     }
>>> +
>>> +     wsc_store_credentials(creds, n_creds);
>>> +     wsc_try_credentials(wsc, creds, n_creds);
>>> +}
>>> +
>>> +static const struct wsc_ops wsc_device_ops = {
>>> +     .get_path = wsc_device_get_path,
>>> +     .connect = wsc_device_connect,
>>> +     .cancel = wsc_device_cancel,
>>> +     .enrollee_done_cb = wsc_device_enrollee_done,
>>> +};
>>> +
>>> +static void wsc_device_new(struct netdev *netdev)
>>>    {
>>>        struct l_dbus *dbus = dbus_get_bus();
>>>        struct wsc *wsc;
>>> @@ -1167,15 +1247,17 @@ static void wsc_add_interface(struct netdev *netdev)
>>>                return;
>>>        }
>>>
>>> -     wsc = l_new(struct wsc, 1);
>>> +     wsc = wsc_new(&wsc_device_ops, NULL);
>>> +     wsc->user_data = wsc;
>>>        wsc->netdev = netdev;
>>>
>>> -     if (!l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
>>> -                                             IWD_WSC_INTERFACE,
>>> -                                             wsc)) {
>>> -             wsc_free(wsc);
>>> -             l_info("Unable to register %s interface", IWD_WSC_INTERFACE);
>>> -     }
>>> +     if (l_dbus_object_add_interface(dbus, netdev_get_path(netdev),
>>> +                                     IWD_WSC_INTERFACE, wsc))
>>> +             return;
>>> +
>>> +     wsc_free(wsc);
>>> +     l_info("Unable to register interface %s at %s", IWD_WSC_INTERFACE,
>>> +             netdev_get_path(netdev));
>>
>> This logic flipping seems gratuitous, I'd just leave it the way it was...
> 
> Ok.
> 
>>
>>>    }
>>>
>>>    static void wsc_remove_interface(struct netdev *netdev)
>>> @@ -1194,7 +1276,7 @@ static void wsc_netdev_watch(struct netdev *netdev,
>>>        case NETDEV_WATCH_EVENT_NEW:
>>>                if (netdev_get_iftype(netdev) == NETDEV_IFTYPE_STATION &&
>>>                                netdev_get_is_up(netdev))
>>> -                     wsc_add_interface(netdev);
>>> +                     wsc_device_new(netdev);
>>>                break;
>>>        case NETDEV_WATCH_EVENT_DOWN:
>>>        case NETDEV_WATCH_EVENT_DEL:
>>> diff --git a/src/wsc.h b/src/wsc.h
>>> index ada08c89..e8a965f2 100644
>>> --- a/src/wsc.h
>>> +++ b/src/wsc.h
>>> @@ -20,6 +20,11 @@
>>>     *
>>>     */
>>>
>>> +struct wsc;
>>> +struct netdev;
>>> +struct scan_bss;
>>> +struct iovec;
>>> +
>>>    struct wsc_credentials_info {
>>>        char ssid[33];
>>>        enum security security;
>>> @@ -31,5 +36,17 @@ struct wsc_credentials_info {
>>>        bool has_passphrase;
>>>    };
>>>
>>> -typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
>>> -                             unsigned int n_creds, void *user_data);
>>> +struct wsc_ops {
>>> +     const char *(*get_path)(void *user_data);
>>> +     void (*connect)(struct l_dbus_message *message, const char *pin,
>>> +                     void *user_data);
>>> +     void (*cancel)(struct l_dbus_message *message, void *user_data);
>>> +     void (*enrollee_done_cb)(int err, struct wsc_credentials_info *creds,
>>> +                                     unsigned int n_creds, void *user_data);
>>
>> So one thing I find very weird about this setup is the fact that you're
>> mixing operations with a callback in something called 'wsc_ops'.  Can't
>> your 'connect' operation simply set the callback or perhaps provide the
>> callback directly to wsc_start_enrollee?
> 
> It would work equally well, yes.  Maybe I was being lazy, I'll fix this.
> 
> One argument for keeping all the callbacks in this struct is that they
> implicitly use the same "user_data" and so we save both the callback
> parameter to wsc_start_enrollee, the user_data parameter (we often
> complained about our functions having too many arguments) and 2 fields
> in struct wsc for storing these two parameters.

Well, so far I only see a single callback being utilized.  So there 
seems to be no need for this just yet?  Also, see my comments above 
about user_data weirdness.

Regards,
-Denis

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 7 Jan 2020 at 17:54, Denis Kenzior <denkenz@gmail.com> wrote:
> On 1/6/20 8:55 PM, Andrew Zaborowski wrote:
> > So in the new setup we have parts of wsc.c that are common between the
> > Station mode use case and the external user use case (i.e. P2P), and
> > then we have parts that are relevant to Station mode only.  We sort of
> > have 3 parts:
> >
> > 1. the core part that sets up the EAP method config, starts the
> > connection and receives the credentials.
> > 2. the DBus interface that lives even when we're not doing WSC (also
> > common to both modes now)
> > 3. the Station-specific code which interfaces between 1. and 2. and
> > does the scanning etc., i.e. everything used by @wsc_device_ops
> >
> > wsc_disconnect_cb belongs in 3., while wsc->wsc_association belongs in
> > 2. so wsc_disconnect_cb shouldn't be touching wsc->wsc_association.
>
> Not quite sure I follow.  wsc_disconnect_cb is called when cancel is
> invoked via D-Bus API (or in your proposal it would be invoked via
> wsc_cancel).  wsc->association tracks whether we have a connection
> established for the purposes of EAP-WSC.  Hence if wsc->association is
> true, we have to disconnect it first.  I see these two as inherently linked?

The disconnect is handled at a lower level, by wsc_cancel.
wsc_disconnect_cb doesn't need to do anything related to the internal
protocol state.

The part 1. exposes an API that is used by 3. in wsc.c, or by the
external caller which also supplies a callback like wsc_disconnect_cb.
That callback also can't mess with variables like wsc->wsc_association
(sitting outside wsc.c it has no way anyway).

>
> >> <snip>
> >>
> >>> @@ -525,40 +523,19 @@ static void wsc_connect(struct wsc *wsc)
> >>>    error:
> >>>        handshake_state_free(hs);
> >>>        wsc_connect_cleanup(wsc);
> >>> -     wsc->done_cb(r, NULL, 0, wsc->done_data);
> >>> +     wsc->ops->enrollee_done_cb(r, NULL, 0, wsc->user_data);
> >>>    }
> >>>
> >>> -/* Done callback for when WSC is triggered by DBus methods */
> >>> -static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
> >>> -                             unsigned int n_creds, void *user_data)
> >>> +static void wsc_connect_set_method(struct wsc *wsc)
> >>>    {
> >>> -     struct wsc *wsc = user_data;
> >>> -
> >>> -     l_debug("err=%i", err);
> >>> +     const char *pin;
> >>>
> >>> -     switch (err) {
> >>> -     case 0:
> >>> -             break;
> >>> -     case -ECANCELED:
> >>> -             dbus_pending_reply(&wsc->pending,
> >>> -                                     dbus_error_aborted(wsc->pending));
> >>> -             return;
> >>> -     case -ENOKEY:
> >>> -             dbus_pending_reply(&wsc->pending,
> >>> -                                     wsc_error_no_credentials(wsc->pending));
> >>> -             return;
> >>> -     case -EBUSY:
> >>> -             dbus_pending_reply(&wsc->pending,
> >>> -                                     dbus_error_busy(wsc->pending));
> >>> -             return;
> >>> -     default:
> >>> -             dbus_pending_reply(&wsc->pending,
> >>> -                                     dbus_error_failed(wsc->pending));
> >>> -             return;
> >>> -     }
> >>> +     if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin"))
> >>> +             l_dbus_message_get_arguments(wsc->pending, "s", &pin);
> >>> +     else
> >>> +             pin = NULL;
> >>>
> >>> -     wsc_store_credentials(wsc);
> >>> -     wsc_try_credentials(wsc);
> >>> +     wsc_start_enrollee(wsc, wsc->netdev, wsc->target, pin, NULL, 0);
> >>
> >> Is wsc_start_enrollee going to be called from p2p directly or only via
> >> the D-Bus API?
> >
> > So the D-Bus API will call the P2P code (to do GO Negotiation /
> > Provision Discovery etc.) or the station-specific code inside wsc.c
> > (to do the scanning) and they'll both call wsc_start_enrollee when
> > ready.  wsc_connect_set_method is the station-specific code path where
> > this happens.
>
> Okay, but the PIN / PushButton setup is common to both, no?  And it
> seems that would logically belong in the overall D-Bus layer handlers.
> No point passing pin around, especially since you already made the
> change to store it inside the wsc object.

Right, I guess the pin can be set directly when the method is called
and cleared whenever we're replying to the method.

> >>> @@ -1001,20 +955,7 @@ static struct l_dbus_message *wsc_push_button(struct l_dbus *dbus,
> >>>
> >>>        l_debug("");
> >>>
> >>> -     if (wsc->pending)
> >>> -             return dbus_error_busy(message);
> >>> -
> >>
> >> So why would you move this check out into the ops provided operation and
> >> not keep it here?
> >>
> > ...
> >>> -     wsc->pending = l_dbus_message_ref(message);
> >>> -
> >>
> >> Pending tracking should also likely be managed here...
> >>
> > ...
> >>> -     if (wsc->pending)
> >>> -             return dbus_error_busy(message);
> >>
> >> Same comments as above apply here ...
> >
> > For one, in the P2P case, it's not going to have access to the
> > internals of struct wsc, so if P2P replies with success or an error
> > message to the PushButton call, it would need some way to also clear
> > wsc->pending.  That's doable but it seems simpler to leave full
> > control to the actual implementation.  So my idea in this version was
> > that the DBus handlers would be really simple and stateless.
> >
>
> But how would p2p set wsc->pending if it has no access to the wsc
> object?  Or are you duplicating the D-Bus handling code inside p2p?  I
> guess I was under the impression that you'd want to unify the D-Bus bits
> as well.

In this case wsc->pending was only part of 3. (out of the three state
machines in wsc.c I mentioned) and p2p.c had it's own "pending" which
is also used as a flag internally.  I guess it makes sense to change
this to handle the dbus message completely in wsc.c so p2p.c doesn't
have to see the actual messages.

>
> If you want to re-invent this inside p2p, then there's really no need
> for this wsc_ops abstraction.  Just have something like:
>
> struct wsc_dbus_object {
>         bool station;
>         union {
>                 struct wsc_p2p *p2p;
>                 struct wsc *wsc;
>         }
> }

I don't follow.  Our entry points are the DBus method handlers, which
are in wsc.c.  How will the control pass from wsc.c to p2p.c (or to
the station specific CBs) without the wsc_ops?

In terms of the structs in wsc.c, yeah, I was thinking of splitting
the state of 1., 2. and 3. into separate structs, something I didn't
do so they're all together in struct wsc now.

>
> > Also remember that in the P2P case the PushButton method will kick off
> > the connection process but there's a few different steps and WSC is
> > just one of them and might be skipped completely in some scenarios.
> >
>
> Sure, so you'd abstract wsc eap connection behind
> wsc_eap_start_enrollee, wsc_eap_cancel, etc.  This ends up being re-used
> and the rest you can do separate.
>
> >>> @@ -1155,7 +1048,194 @@ static void wsc_free(void *userdata)
> >>>        l_free(wsc);
> >>>    }
> >>>
> >>> -static void wsc_add_interface(struct netdev *netdev)
> >>> +struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data)
> >>> +{
> >>> +     struct l_dbus *dbus = dbus_get_bus();
> >>> +     struct wsc *wsc;
> >>> +     const char *path;
> >>> +
> >>> +     wsc = l_new(struct wsc, 1);
> >>> +     wsc->ops = ops;
> >>> +     wsc->user_data = user_data;
> >>
> >> This part seems rather weird
> >
> > Do you mean the whole function? and why?
>
> I really meant the user_data setting.  You pass in user_data to all the
> callbacks, but it is set to NULL in wsc_new.  I think the intent was for
> user_data to be the wsc object itself, but I don't see how that would
> even be accomplished in this setup.  Have you actually tested this patch
> standalone?

I think I did (this was a month ago) ;-)  You're right, the idea is
for the user_data to be the wsc object itself because I didn't split
the state into separate structs.  So in the station mode case
wsc_add_interface calls wsc_new, then overwrites wsc->user_data.

>
> >>> @@ -31,5 +36,17 @@ struct wsc_credentials_info {
> >>>        bool has_passphrase;
> >>>    };
> >>>
> >>> -typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
> >>> -                             unsigned int n_creds, void *user_data);
> >>> +struct wsc_ops {
> >>> +     const char *(*get_path)(void *user_data);
> >>> +     void (*connect)(struct l_dbus_message *message, const char *pin,
> >>> +                     void *user_data);
> >>> +     void (*cancel)(struct l_dbus_message *message, void *user_data);
> >>> +     void (*enrollee_done_cb)(int err, struct wsc_credentials_info *creds,
> >>> +                                     unsigned int n_creds, void *user_data);
> >>
> >> So one thing I find very weird about this setup is the fact that you're
> >> mixing operations with a callback in something called 'wsc_ops'.  Can't
> >> your 'connect' operation simply set the callback or perhaps provide the
> >> callback directly to wsc_start_enrollee?
> >
> > It would work equally well, yes.  Maybe I was being lazy, I'll fix this.
> >
> > One argument for keeping all the callbacks in this struct is that they
> > implicitly use the same "user_data" and so we save both the callback
> > parameter to wsc_start_enrollee, the user_data parameter (we often
> > complained about our functions having too many arguments) and 2 fields
> > in struct wsc for storing these two parameters.
>
> Well, so far I only see a single callback being utilized.  So there
> seems to be no need for this just yet?

.connect and .cancel are both being utilized, and .enrollee_done_cb is
also being utilized but at a different level.  There's currently one
user_data for them.

What I meant is that if I move enrollee_done_cb outside of the "ops"
it's going to be part of the wsc instance state and it probably should
have its own user_data so those are 2 new parameters and 2 new
variables.  But it may be worth it anyway.

Best regards

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Denis Kenzior]

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

Hi Andrew,

<snip>

> The disconnect is handled at a lower level, by wsc_cancel.
> wsc_disconnect_cb doesn't need to do anything related to the internal
> protocol state.
> 
> The part 1. exposes an API that is used by 3. in wsc.c, or by the
> external caller which also supplies a callback like wsc_disconnect_cb.
> That callback also can't mess with variables like wsc->wsc_association
> (sitting outside wsc.c it has no way anyway).
> 

Okay, so I had to stare at this again for a while and I think I get it 
now.  Note that you only call wsc_connect_cleanup if netdev_disconnect 
fails.  So wsc_connect_cleanup is not called unnecessarily.

<snip>

> 
> In this case wsc->pending was only part of 3. (out of the three state
> machines in wsc.c I mentioned) and p2p.c had it's own "pending" which
> is also used as a flag internally.  I guess it makes sense to change
> this to handle the dbus message completely in wsc.c so p2p.c doesn't
> have to see the actual messages.

If it was me doing it, I'd try to unify the D-Bus API handling aspects. 
Especially since you went to the trouble of abstracting all the operations.

> 
>>
>> If you want to re-invent this inside p2p, then there's really no need
>> for this wsc_ops abstraction.  Just have something like:
>>
>> struct wsc_dbus_object {
>>          bool station;
>>          union {
>>                  struct wsc_p2p *p2p;
>>                  struct wsc *wsc;
>>          }
>> }
> 
> I don't follow.  Our entry points are the DBus method handlers, which
> are in wsc.c.  How will the control pass from wsc.c to p2p.c (or to
> the station specific CBs) without the wsc_ops?

it would just invoke the relevant function calls directly.  i.e.

wsc_p2p_push_button(p2p, msg);
wsc_p2p_start_pin(p2p, msg);
wsc_p2p_cancel(p2p, msg);

That would give you a whole lot more freedom to do whatever in p2p.

> 
> I think I did (this was a month ago) ;-)  You're right, the idea is
> for the user_data to be the wsc object itself because I didn't split
> the state into separate structs.  So in the station mode case
> wsc_add_interface calls wsc_new, then overwrites wsc->user_data.

Yep, I think I missed the part of you overwriting the user_data.  You 
went through all this trouble of defining an API for setting it and then 
turn around and backdoor it ;)  That is what threw me off here and also 
in the disconnect_cb bits above.  If user_data was meant to be set 
externally, it would have been clearer to add wsc_set_userdata() method 
or have the caller pass in the cb/userdata into the operation directly.

> 
> .connect and .cancel are both being utilized, and .enrollee_done_cb is
> also being utilized but at a different level.  There's currently one
> user_data for them.

So while this works, I would think that separating the layers more 
explicitly would make things more elegant.  You have the EAP-WSC state 
machine object that takes care of the actual WSC connection:
	wsc_enrollee_new
	wsc_enrollee_start
	wsc_enrollee_cancel
	wsc_enrollee_free

Then you have the p2p/station layers that take care of scanning, etc. 
This is where your ops stuff can live.  Something like:

struct wsc {
	const char *(*get_path)(struct wsc *wsc);
	int (*connect)(struct wsc *wsc, const char *pin,
			connect_cb_t cb, void *userdata);
	int (*cancel)(struct wsc *wsc, cancel_cb_t cb, void *userdata);
	void (*remove)(struct wsc *wsc);
};

Then for p2p you can do:

struct wsc_p2p {
	// some members
	struct wsc super;
};

struct wsc *wsc_p2p_new();

for station:
struct wsc_station {
	struct wsc super;
	// some members;
	struct station *station;
	uint32_t station_state_watch;
	...
};

struct wsc *wsc_station_new();

And if you want to unify the D-Bus bits, then:

struct wsc_dbus {
	struct wsc *wsc;
	struct l_dbus_message *pending;
	struct l_dbus_message *pending_cancel;
};
	
Regards,
-Denis

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 9 Jan 2020 at 18:21, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
> <snip>
>
> >
> > In this case wsc->pending was only part of 3. (out of the three state
> > machines in wsc.c I mentioned) and p2p.c had it's own "pending" which
> > is also used as a flag internally.  I guess it makes sense to change
> > this to handle the dbus message completely in wsc.c so p2p.c doesn't
> > have to see the actual messages.
>
> If it was me doing it, I'd try to unify the D-Bus API handling aspects.
> Especially since you went to the trouble of abstracting all the operations.
>
> >
> >>
> >> If you want to re-invent this inside p2p, then there's really no need
> >> for this wsc_ops abstraction.  Just have something like:
> >>
> >> struct wsc_dbus_object {
> >>          bool station;
> >>          union {
> >>                  struct wsc_p2p *p2p;
> >>                  struct wsc *wsc;
> >>          }
> >> }
> >
> > I don't follow.  Our entry points are the DBus method handlers, which
> > are in wsc.c.  How will the control pass from wsc.c to p2p.c (or to
> > the station specific CBs) without the wsc_ops?
>
> it would just invoke the relevant function calls directly.  i.e.
>
> wsc_p2p_push_button(p2p, msg);
> wsc_p2p_start_pin(p2p, msg);
> wsc_p2p_cancel(p2p, msg);
>
> That would give you a whole lot more freedom to do whatever in p2p.

I think I'd rather use the ops struct defined in wsc.h than expose
these functions in p2p.h.  A strict P2P API would look differently so
this is shaped after WSC.  It also needs the user-data to know which
peer to connect to.

>
> >
> > I think I did (this was a month ago) ;-)  You're right, the idea is
> > for the user_data to be the wsc object itself because I didn't split
> > the state into separate structs.  So in the station mode case
> > wsc_add_interface calls wsc_new, then overwrites wsc->user_data.
>
> Yep, I think I missed the part of you overwriting the user_data.  You
> went through all this trouble of defining an API for setting it and then
> turn around and backdoor it ;)

True.  Yep, I should've added a wsc_set_userdata.

> That is what threw me off here and also
> in the disconnect_cb bits above.  If user_data was meant to be set
> externally, it would have been clearer to add wsc_set_userdata() method
> or have the caller pass in the cb/userdata into the operation directly.
>
> >
> > .connect and .cancel are both being utilized, and .enrollee_done_cb is
> > also being utilized but at a different level.  There's currently one
> > user_data for them.
>
> So while this works, I would think that separating the layers more
> explicitly would make things more elegant.  You have the EAP-WSC state
> machine object that takes care of the actual WSC connection:
>         wsc_enrollee_new
>         wsc_enrollee_start
>         wsc_enrollee_cancel
>         wsc_enrollee_free

Ok, this sounds good.  I'm not even sure if we need a start and a
cancel, or it's Ok to create and free this whole state machine as
needed.

So those functions would operate on a "struct wsc_enrollee".

>
> Then you have the p2p/station layers that take care of scanning, etc.
> This is where your ops stuff can live.  Something like:
>
> struct wsc {
>         const char *(*get_path)(struct wsc *wsc);
>         int (*connect)(struct wsc *wsc, const char *pin,
>                         connect_cb_t cb, void *userdata);
>         int (*cancel)(struct wsc *wsc, cancel_cb_t cb, void *userdata);
>         void (*remove)(struct wsc *wsc);
> };

So this would be exposed in wsc.h I understand.  I think I'd still
rather call it wsc_ops.

>
> Then for p2p you can do:
>
> struct wsc_p2p {
>         // some members
>         struct wsc super;
> };
>
> struct wsc *wsc_p2p_new();

And this would live in p2p.c.  We have one WSC interface per peer so
this would really be called "struct p2p_peer_wsc_dbus" or similar and
in the callbacks it can use container_of() instead of needing the
user_data so that parameter can be dropped.

>
> for station:
> struct wsc_station {
>         struct wsc super;
>         // some members;
>         struct station *station;
>         uint32_t station_state_watch;
>         ...
> };
>
> struct wsc *wsc_station_new();
>
> And if you want to unify the D-Bus bits, then:
>
> struct wsc_dbus {
>         struct wsc *wsc;
>         struct l_dbus_message *pending;
>         struct l_dbus_message *pending_cancel;
> };

Ok, so this would be created by a call to:

struct wsc_dbus *wsc_dbus_new(struct wsc *wsc)   (or struct wsc_ops *wsc)

and it would either need to be public in wsc.h or have enough
accessors for pending and pending_cancel.

Best regards

Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Denis Kenzior]

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

Hi Andrew,

>> So while this works, I would think that separating the layers more
>> explicitly would make things more elegant.  You have the EAP-WSC state
>> machine object that takes care of the actual WSC connection:
>>          wsc_enrollee_new
>>          wsc_enrollee_start
>>          wsc_enrollee_cancel
>>          wsc_enrollee_free
> 
> Ok, this sounds good.  I'm not even sure if we need a start and a
> cancel, or it's Ok to create and free this whole state machine as
> needed.

See how it looks.  We might need a cancel at least since you have a 
'destroy immediately' case where the device goes away due to hot-unplug. 
  Or cancel and try to cleanup first case.

> 
> So those functions would operate on a "struct wsc_enrollee".

Right.

> 
>>
>> Then you have the p2p/station layers that take care of scanning, etc.
>> This is where your ops stuff can live.  Something like:
>>
>> struct wsc {
>>          const char *(*get_path)(struct wsc *wsc);
>>          int (*connect)(struct wsc *wsc, const char *pin,
>>                          connect_cb_t cb, void *userdata);
>>          int (*cancel)(struct wsc *wsc, cancel_cb_t cb, void *userdata);
>>          void (*remove)(struct wsc *wsc);
>> };
> 
> So this would be exposed in wsc.h I understand.  I think I'd still
> rather call it wsc_ops.
> 

Can do that too.  But there might be state you can share across the 
different sub-types.  Maybe the netdev for example.

>>
>> Then for p2p you can do:
>>
>> struct wsc_p2p {
>>          // some members
>>          struct wsc super;
>> };
>>
>> struct wsc *wsc_p2p_new();
> 
> And this would live in p2p.c.  We have one WSC interface per peer so
> this would really be called "struct p2p_peer_wsc_dbus" or similar and
> in the callbacks it can use container_of() instead of needing the
> user_data so that parameter can be dropped.
> 

Yep

>>
>> for station:
>> struct wsc_station {
>>          struct wsc super;
>>          // some members;
>>          struct station *station;
>>          uint32_t station_state_watch;
>>          ...
>> };
>>
>> struct wsc *wsc_station_new();
>>
>> And if you want to unify the D-Bus bits, then:
>>
>> struct wsc_dbus {
>>          struct wsc *wsc;
>>          struct l_dbus_message *pending;
>>          struct l_dbus_message *pending_cancel;
>> };
> 
> Ok, so this would be created by a call to:
> 
> struct wsc_dbus *wsc_dbus_new(struct wsc *wsc)   (or struct wsc_ops *wsc)
> 

yes, something like that would be needed.  Can also do something like 
wsc_dbus_object_add and let struct wsc_dbus be hidden away in wsc.c

> and it would either need to be public in wsc.h or have enough
> accessors for pending and pending_cancel.
> 

Reegards,
-Denis

[PATCH 4/4] wsc: Accept extra IEs in wsc_start_enrollee

[Andrew Zaborowski]

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

---
 src/wsc.c | 24 ++++++++++++++----------
 src/wsc.h |  3 ++-
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/wsc.c b/src/wsc.c
index 1901feb6..423d2aa8 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -447,7 +447,8 @@ static inline enum wsc_rf_band freq_to_rf_band(uint32_t freq)
 	return WSC_RF_BAND_2_4_GHZ;
 }
 
-static void wsc_connect(struct wsc *wsc)
+static void wsc_connect(struct wsc *wsc, struct iovec *ies,
+			unsigned int ies_num)
 {
 	struct handshake_state *hs;
 	struct l_settings *settings = l_settings_new();
@@ -456,7 +457,7 @@ static void wsc_connect(struct wsc *wsc)
 	struct wsc_association_request request;
 	uint8_t *pdu;
 	size_t pdu_len;
-	struct iovec ie_iov;
+	struct iovec ie_iov[1 + ies_num];
 
 	wsc->target = NULL;
 
@@ -502,18 +503,20 @@ static void wsc_connect(struct wsc *wsc)
 		goto error;
 	}
 
-	ie_iov.iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
-							&ie_iov.iov_len);
+	ie_iov[0].iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
+							&ie_iov[0].iov_len);
 	l_free(pdu);
 
-	if (!ie_iov.iov_base) {
+	if (!ie_iov[0].iov_base) {
 		r = -ENOMEM;
 		goto error;
 	}
 
-	r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
-				wsc_connect_cb, wsc);
-	l_free(ie_iov.iov_base);
+	memcpy(ie_iov + 1, ies, sizeof(struct iovec) * ies_num);
+
+	r = netdev_connect(wsc->netdev, bss, hs, ie_iov, 1 + ies_num,
+				wsc_netdev_event, wsc_connect_cb, wsc);
+	l_free(ie_iov[0].iov_base);
 
 	if (r < 0)
 		goto error;
@@ -1079,7 +1082,8 @@ void wsc_destroy(struct wsc *wsc)
 }
 
 void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
-			struct scan_bss *target, const char *pin)
+			struct scan_bss *target, const char *pin,
+			struct iovec *ies, unsigned int ies_num)
 {
 	wsc->netdev = netdev;
 	wsc->target = target;
@@ -1087,7 +1091,7 @@ void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
 		WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
 	wsc->pin = l_strdup(pin);
 
-	wsc_connect(wsc);
+	wsc_connect(wsc, ies, ies_num);
 }
 
 void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb)
diff --git a/src/wsc.h b/src/wsc.h
index e8a965f2..73ffdd33 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -47,6 +47,7 @@ struct wsc_ops {
 
 struct wsc *wsc_new(const struct wsc_ops *ops, void *user_data);
 void wsc_start_enrollee(struct wsc *wsc, struct netdev *netdev,
-			struct scan_bss *target, const char *pin);
+			struct scan_bss *target, const char *pin,
+			struct iovec *ies, unsigned int ies_num);
 void wsc_cancel(struct wsc *wsc, netdev_disconnect_cb_t cb);
 void wsc_destroy(struct wsc *wsc);
-- 
2.20.1

Re: [PATCH 1/4] netdev: Replace bool randomize_mac with specific address

[Denis Kenzior]

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

Hi Andrew,

On 12/18/19 9:50 PM, Andrew Zaborowski wrote:
> Allow netdev_create_from_genl callers to draw a random or non-random MAC
> and pass it in the parameter instead of a bool to tell us to generating
> the MAC locally.  In P2P we are generating the MAC some time before
> creating the netdev in order to pass it to the peer during negotiation.
> ---
>   src/manager.c | 22 ++++++++++++++++++----
>   src/netdev.c  | 45 ++++++++++++++++++++++-----------------------
>   src/netdev.h  |  3 ++-
>   3 files changed, 42 insertions(+), 28 deletions(-)
> 

Applied, thanks.

Regards,
-Denis