[PATCH 01/12] doc: Fix p2p method error names

[PATCH 01/12] doc: Fix p2p method error names

[Andrew Zaborowski]

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

---
 doc/p2p-api.txt      | 2 +-
 doc/p2p-peer-api.txt | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/doc/p2p-api.txt b/doc/p2p-api.txt
index cca0f39f..08ee8d92 100644
--- a/doc/p2p-api.txt
+++ b/doc/p2p-api.txt
@@ -40,7 +40,7 @@ Methods		array(on) GetPeers()
 			discovered peer devices to avoid keeping the Wi-Fi
 			adapter occupied unnecessarily.
 
-			Possible errors: [service].Error.NotAvailable
+			Possible errors: [service].Error.NotFound
 
 		void RegisterSignalLevelAgent(object path,
 						 array(int16) levels)
diff --git a/doc/p2p-peer-api.txt b/doc/p2p-peer-api.txt
index 2245b2e3..f52717aa 100644
--- a/doc/p2p-peer-api.txt
+++ b/doc/p2p-peer-api.txt
@@ -12,8 +12,8 @@ Methods		Disconnect()
 			the net.connman.iwd.SimpleConfiguration interface
 			on the same object -- see wsc-api.txt.
 
-			Possible errors: [service].Failed
-					 [service].NotConnected
+			Possible errors: [service].Error.Failed
+					 [service].Error.NotConnected
 
 Properties	string Name [readonly]
 
-- 
2.20.1

[PATCH 02/12] cleanup: Remove extra empty lines

[Andrew Zaborowski]

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

---
 src/eap-mschapv2.h | 1 -
 src/eap-pwd.c      | 1 -
 src/ie.c           | 1 -
 3 files changed, 3 deletions(-)

diff --git a/src/eap-mschapv2.h b/src/eap-mschapv2.h
index 6b090969..d1f4fd53 100644
--- a/src/eap-mschapv2.h
+++ b/src/eap-mschapv2.h
@@ -28,7 +28,6 @@
 #include <stdint.h>
 #include <stdbool.h>
 
-
 bool mschapv2_get_asymmetric_start_key(const uint8_t master_key[static 16],
 				uint8_t *session_key, size_t session_len,
 				bool server, bool send);
diff --git a/src/eap-pwd.c b/src/eap-pwd.c
index 74d0997f..a77f21b9 100644
--- a/src/eap-pwd.c
+++ b/src/eap-pwd.c
@@ -435,7 +435,6 @@ error:
 	eap_method_error(eap);
 }
 
-
 static void eap_pwd_handle_confirm(struct eap_state *eap,
 					const uint8_t *pkt, size_t len)
 {
diff --git a/src/ie.c b/src/ie.c
index 9b1127ce..f120c89e 100644
--- a/src/ie.c
+++ b/src/ie.c
@@ -2406,7 +2406,6 @@ int ie_parse_neighbor_report(struct ie_tlv_iter *iter,
 	return 0;
 }
 
-
 int ie_parse_roaming_consortium(struct ie_tlv_iter *iter, size_t *num_anqp_out,
 				const uint8_t **oi1_out, size_t *oi1_len_out,
 				const uint8_t **oi2_out, size_t *oi2_len_out,
-- 
2.20.1

[PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback

[Andrew Zaborowski]

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

Don't try to handle l_io read errors in the read handler destroy
callback.  That cb is called both when the read handler returns false,
and when poll returns an error.  In neither case the l_io is
automatically destroyed so we were wrongly setting group->io = NULL,
we need to manually destroy the l_io.  But you can't use l_io_destroy
safely from inside the read handler destroy callback.  Do this in the
disconnect callback instead which is safe.
---
 src/frame-xchg.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index ce69c454..3719ed6c 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -234,6 +234,7 @@ static void frame_watch_group_destroy(void *data)
 		l_genl_remove_unicast_watch(iwd_get_genl(),
 						group->unicast_watch_id);
 
+	l_io_set_disconnect_handler(group->io, NULL, NULL, NULL);
 	l_io_destroy(group->io);
 	l_queue_destroy(group->write_queue,
 			(l_queue_destroy_func_t) l_genl_msg_unref);
@@ -359,17 +360,16 @@ static bool frame_watch_group_io_read(struct l_io *io, void *user_data)
 	return true;
 }
 
-static void frame_watch_group_io_destroy(void *user_data)
+static void frame_watch_group_io_disconnect(struct l_io *io, void *user_data)
 {
 	struct watch_group *group = user_data;
 
-	group->io = NULL;
+	if (!l_queue_remove(watch_groups, group))
+		return;
 
-	if (l_queue_remove(watch_groups, group)) {
-		l_error("Frame watch group socket closed");
+	l_error("Frame watch group %i socket closed", (int) group->id);
 
-		frame_watch_group_destroy(group);
-	}
+	frame_watch_group_destroy(group);
 }
 
 static struct watch_group *frame_watch_group_new(uint64_t wdev_id, uint32_t id)
@@ -432,8 +432,10 @@ static struct watch_group *frame_watch_group_new(uint64_t wdev_id, uint32_t id)
 		goto err;
 
 	l_io_set_close_on_destroy(group->io, true);
-	l_io_set_read_handler(group->io, frame_watch_group_io_read, group,
-				frame_watch_group_io_destroy);
+	l_io_set_read_handler(group->io, frame_watch_group_io_read,
+					group, NULL);
+	l_io_set_disconnect_handler(group->io, frame_watch_group_io_disconnect,
+					group, NULL);
 	group->write_queue = l_queue_new();
 
 	return group;
-- 
2.20.1

[PATCH 04/12] frame-xchg: Actually free duplicate watches

[Andrew Zaborowski]

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

Fix a potential leak when we need to drop an existing watch because it's
being replaced with a new one.
---
 src/frame-xchg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 3719ed6c..68bd6168 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -524,6 +524,7 @@ static bool frame_watch_check_duplicate(void *data, void *user_data)
 	}
 
 	/* Drop the existing watch as a duplicate of the new one */
+	frame_watch_free(&watch->super);
 	return true;
 }
 
-- 
2.20.1

[PATCH 05/12] frame-xchg: Fix frame_watch_item_remove_by_handler

[Andrew Zaborowski]

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

---
 src/frame-xchg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 68bd6168..7b45ebb6 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -658,7 +658,8 @@ struct frame_watch_handler_check_info {
 
 static bool frame_watch_item_remove_by_handler(void *data, void *user_data)
 {
-	struct frame_watch *watch = data;
+	struct frame_watch *watch =
+		l_container_of(data, struct frame_watch, super);
 	struct frame_watch_handler_check_info *info = user_data;
 
 	if (watch->super.notify != info->handler ||
-- 
2.20.1

[PATCH 06/12] frame-xchg: Allow calling frame_xchg_stop from the callback

[Andrew Zaborowski]

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

Don't crash if the user calls frame_xchg_stop(wdev) from inside the
frame exchange's final callback.  That call is going to be redundant but
it's convenient to do this inside a cleanup function for a given wdev
without having to check whether any frame exchange was actually running.
---
 src/frame-xchg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 7b45ebb6..7b66cb6d 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -1268,9 +1268,10 @@ error:
 static void frame_xchg_exit(void)
 {
 	struct l_queue *groups = watch_groups;
+	struct l_queue *xchgs = frame_xchgs;
 
-	l_queue_destroy(frame_xchgs, frame_xchg_cancel);
 	frame_xchgs = NULL;
+	l_queue_destroy(xchgs, frame_xchg_cancel);
 
 	watch_groups = NULL;
 	l_queue_destroy(groups, frame_watch_group_destroy);
-- 
2.20.1

[PATCH 07/12] frame-xchg: Allow frame_xchg_stop calls inside frame callbacks

[Andrew Zaborowski]

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

Make sure a frame callback is free to call frame_xchg_stop without
causing a crash.  Frame callback here means the one that gets
called if our tx frame was ACKed and triggered a respone frame that
matched one of the provided prefixes, within the given time.

All in all a frame callback is allowed to call either
frame_xchg_stop or frame_xchg_startv or neither.  Same applies to
the final callback (called when no matching responses received).
---
 src/frame-xchg.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index 7b66cb6d..dd6d63ad 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -108,6 +108,7 @@ struct frame_xchg_data {
 	unsigned int retry_interval;
 	unsigned int resp_timeout;
 	bool in_frame_cb : 1;
+	bool stale : 1;
 };
 
 struct frame_xchg_watch_data {
@@ -725,6 +726,7 @@ static void frame_xchg_wait_cancel(struct frame_xchg_data *fx)
 static void frame_xchg_reset(struct frame_xchg_data *fx)
 {
 	fx->in_frame_cb = false;
+	fx->stale = false;
 
 	frame_xchg_wait_cancel(fx);
 
@@ -972,7 +974,7 @@ static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
 
 		fx->in_frame_cb = false;
 
-		if (done) {
+		if (done || fx->stale) {
 			fx->cb = NULL;
 			frame_xchg_done(fx, 0);
 			return;
@@ -1117,6 +1119,11 @@ void frame_xchg_stop(uint64_t wdev_id)
 	if (!fx)
 		return;
 
+	if (fx->in_frame_cb) {
+		fx->stale = true;
+		return;
+	}
+
 	frame_xchg_reset(fx);
 	l_free(fx);
 }
-- 
2.20.1

[PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy

[Andrew Zaborowski]

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

If during WATCHLIST_NOTIFY{,_MATCHES,_NO_ARGS} one of the watch
notify callback triggers a call to watchlist_destroy, give up calling
remaining watches and destroy the watchlist without crashing.  This is
useful in frame-xchg.c (P2P use case) where a frame watch may trigger
a move to a new state after receiving a specific frame, and remove one
group of frame watches (including its watchlist) to create a different
group.
---
 src/watchlist.c |  5 +++++
 src/watchlist.h | 22 ++++++++++++++++++----
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/src/watchlist.c b/src/watchlist.c
index 4f4d8259..a14476bd 100644
--- a/src/watchlist.c
+++ b/src/watchlist.c
@@ -127,6 +127,11 @@ static void __watchlist_clear(struct watchlist *watchlist)
 
 void watchlist_destroy(struct watchlist *watchlist)
 {
+	if (watchlist->in_notify) {
+		watchlist->pending_destroy = true;
+		return;
+	}
+
 	__watchlist_clear(watchlist);
 	l_queue_destroy(watchlist->items, NULL);
 	watchlist->items = NULL;
diff --git a/src/watchlist.h b/src/watchlist.h
index cd408ac8..c3632859 100644
--- a/src/watchlist.h
+++ b/src/watchlist.h
@@ -38,6 +38,7 @@ struct watchlist {
 	struct l_queue *items;
 	bool in_notify : 1;
 	bool stale_items : 1;
+	bool pending_destroy : 1;
 	const struct watchlist_ops *ops;
 };
 
@@ -69,9 +70,13 @@ void __watchlist_prune_stale(struct watchlist *watchlist);
 			if (item->id == 0)				\
 				continue;				\
 			t(args, item->notify_data);			\
+			if ((watchlist)->pending_destroy)		\
+				break;					\
 		}							\
 		(watchlist)->in_notify = false;				\
-		if ((watchlist)->stale_items)				\
+		if ((watchlist)->pending_destroy)			\
+			watchlist_destroy(watchlist);			\
+		else if ((watchlist)->stale_items)			\
 			__watchlist_prune_stale(watchlist);		\
 	} while	(false)							\
 
@@ -91,9 +96,14 @@ void __watchlist_prune_stale(struct watchlist *watchlist);
 				continue;				\
 									\
 			t(args, item->notify_data);			\
+									\
+			if ((watchlist)->pending_destroy)		\
+				break;					\
 		}							\
 		(watchlist)->in_notify = false;				\
-		if ((watchlist)->stale_items)				\
+		if ((watchlist)->pending_destroy)			\
+			watchlist_destroy(watchlist);			\
+		else if ((watchlist)->stale_items)			\
 			__watchlist_prune_stale(watchlist);		\
 	} while	(false)
 
@@ -108,9 +118,13 @@ void __watchlist_prune_stale(struct watchlist *watchlist);
 			type t = item->notify;				\
 			if (item->id == 0)				\
 				continue;				\
-			t(item->notify_data);			\
+			t(item->notify_data);				\
+			if ((watchlist)->pending_destroy)		\
+				break;					\
 		}							\
 		(watchlist)->in_notify = false;				\
-		if ((watchlist)->stale_items)				\
+		if ((watchlist)->pending_destroy)			\
+			watchlist_destroy(watchlist);			\
+		else if ((watchlist)->stale_items)			\
 			__watchlist_prune_stale(watchlist);		\
 	} while	(false)
-- 
2.20.1

[PATCH 09/12] frame-xchg: Optimize frame_watch_remove_by_handler scenarios

[Andrew Zaborowski]

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

Since frame_watch_remove_by_handler only forgets a given function +
user data pointers, and doesn't remove the frame prefixes added in the
kernel, we can avoid later re-registering those prefixes with the
kernel by keeping them in our local watchlist, and only replacing the
handler pointer with a dummy function.
---
 src/frame-xchg.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index dd6d63ad..a7565a24 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -474,6 +474,12 @@ static void frame_watch_register_cb(struct l_genl_msg *msg, void *user_data)
 			L_PTR_TO_UINT(user_data), l_genl_msg_get_error(msg));
 }
 
+static void frame_watch_notify_empty(const struct mmpdu_header *mpdu,
+					const void *body, size_t body_len,
+					int rssi, void *user_data)
+{
+}
+
 struct frame_duplicate_info {
 	uint64_t wdev_id;
 	uint16_t frame_type;
@@ -508,6 +514,17 @@ static bool frame_watch_check_duplicate(void *data, void *user_data)
 		 */
 		info->registered = true;
 
+	/*
+	 * If we previously had a watch registered on this socket,
+	 * with the same or a more specific prefix, we can now forget
+	 * its entry as the new watch is going to hold enough
+	 * information to keep us from registering redundant prefixes
+	 * in the future.
+	 */
+	if (info->prefix_len <= watch->prefix_len &&
+			watch->super.notify == frame_watch_notify_empty)
+		goto drop;
+
 	if (info->handler != watch->super.notify ||
 			info->user_data != watch->super.notify_data)
 		return false;
@@ -524,6 +541,7 @@ static bool frame_watch_check_duplicate(void *data, void *user_data)
 		return false;
 	}
 
+drop:
 	/* Drop the existing watch as a duplicate of the new one */
 	frame_watch_free(&watch->super);
 	return true;
@@ -667,11 +685,19 @@ static bool frame_watch_item_remove_by_handler(void *data, void *user_data)
 			watch->super.notify_data != info->user_data)
 		return false;
 
-	if (watch->super.destroy)
+	if (watch->super.destroy) {
 		watch->super.destroy(watch->super.notify_data);
+		watch->super.destroy = NULL;
+	}
 
-	frame_watch_free(&watch->super);
-	return true;
+	/*
+	 * Keep the entry, with a dummy callback, in order to keep us from
+	 * re-registering its prefix in future frame_watch_add calls.  We
+	 * can drop the entry in some circumstances but checking the
+	 * conditions for this costs more than it is worth right now.
+	 */
+	watch->super.notify = frame_watch_notify_empty;
+	return false;
 }
 
 /*
-- 
2.20.1

[PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK

[Andrew Zaborowski]

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

In some cases a P2P peer will ACK our frame but not reply on the first
attempt, and other implementations seem to handle this by going back to
retransmitting the frame at a high rate until it gets ACKed again, at
which point they will again give the peer a longer time to tx the
response frame.  Implement the same logic here by adding a
retries_on_ack parameter that takes the number of additional times we
want to restart the normal retransmit counter after we received no
response frame on the first attempt.  So passing 0 maintains the
current behaviour, 1 for 1 extra attempt, etc.

In effect we may retransmit a frame about 15 * (retry_on_ack + 1) *
<in-kernel retransmit limit> times.  The kernel/driver retransmits a
frame a number of times if there's no ACK (I've seen about 20 normally)
at a high frequency, if that fails we retry the whole process 15 times
inside frame-xchg.c and if we still get no ACK at any point, we give up.
If we do get an ACK, we wait for a response frame and if we don't get
that we will optionally reset the retry counter and restart the whole
thing retry_on_ack times.
---
 src/frame-xchg.c | 61 +++++++++++++++++++++++++++++++++---------------
 src/frame-xchg.h |  4 ++--
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index a7565a24..a4b6b740 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -107,6 +107,7 @@ struct frame_xchg_data {
 	unsigned int retry_cnt;
 	unsigned int retry_interval;
 	unsigned int resp_timeout;
+	unsigned int retries_on_ack;
 	bool in_frame_cb : 1;
 	bool stale : 1;
 };
@@ -722,12 +723,16 @@ static bool frame_watch_remove_by_handler(uint64_t wdev_id, uint32_t group_id,
 	if (!group)
 		return false;
 
+	l_error(" ** settign grp %i elems that match to nop", group->id);////
 	return l_queue_foreach_remove(group->watches.items,
 					frame_watch_item_remove_by_handler,
 					&handler_info) > 0;
 }
 
 static void frame_xchg_tx_retry(struct frame_xchg_data *fx);
+static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
+					const void *body, size_t body_len,
+					int rssi, void *user_data);
 static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
 				const void *body, size_t body_len,
 				int rssi, void *user_data);
@@ -807,12 +812,19 @@ static void frame_xchg_timeout_cb(struct l_timeout *timeout,
 	frame_xchg_tx_retry(fx);
 }
 
-static void frame_xchg_resp_timeout_cb(struct l_timeout *timeout,
+static void frame_xchg_listen_end_cb(struct l_timeout *timeout,
 					void *user_data)
 {
 	struct frame_xchg_data *fx = user_data;
 
-	frame_xchg_done(fx, 0);
+	if (!fx->retries_on_ack--) {
+		frame_xchg_done(fx, 0);
+		return;
+	}
+
+	l_timeout_remove(fx->timeout);
+	fx->retry_cnt = 0;
+	frame_xchg_tx_retry(fx);
 }
 
 static void frame_xchg_tx_status(struct frame_xchg_data *fx, bool acked)
@@ -852,17 +864,19 @@ static void frame_xchg_tx_status(struct frame_xchg_data *fx, bool acked)
 		fx->have_cookie = false;
 
 		l_debug("Processing an early frame");
-		frame_xchg_resp_cb(fx->early_frame.mpdu, fx->early_frame.body,
-					fx->early_frame.body_len,
-					fx->early_frame.rssi, fx);
 
-		frame_xchg_done(fx, 0);
+		if (frame_xchg_resp_handle(fx->early_frame.mpdu, fx->early_frame.body,
+						fx->early_frame.body_len,
+						fx->early_frame.rssi, fx))
+			return;
+
+		frame_xchg_listen_end_cb(NULL, fx);
 		return;
 	}
 
 	/* Txed frame ACKed, listen for response frames */
 	fx->timeout = l_timeout_create_ms(fx->resp_timeout,
-						frame_xchg_resp_timeout_cb, fx,
+						frame_xchg_listen_end_cb, fx,
 						frame_xchg_timeout_destroy);
 }
 
@@ -948,9 +962,9 @@ static void frame_xchg_tx_retry(struct frame_xchg_data *fx)
 	fx->retry_cnt++;
 }
 
-static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
-				const void *body, size_t body_len,
-				int rssi, void *user_data)
+static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
+					const void *body, size_t body_len,
+					int rssi, void *user_data)
 {
 	struct frame_xchg_data *fx = user_data;
 	const struct l_queue_entry *entry;
@@ -959,10 +973,10 @@ static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
 	l_debug("");
 
 	if (memcmp(mpdu->address_1, fx->tx_mpdu->address_2, 6))
-		return;
+		return false;
 
 	if (memcmp(mpdu->address_2, fx->tx_mpdu->address_1, 6))
-		return;
+		return false;
 
 	/*
 	 * Is the received frame's BSSID same as the transmitted frame's
@@ -972,7 +986,7 @@ static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
 	 */
 	if (memcmp(mpdu->address_3, fx->tx_mpdu->address_3, 6) &&
 			!util_mem_is_zero(mpdu->address_3, 6))
-		return;
+		return false;
 
 	for (entry = l_queue_get_entries(fx->rx_watches);
 			entry; entry = entry->next) {
@@ -996,18 +1010,18 @@ static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
 		 * to just exit without touching anything.
 		 */
 		if (!fx->in_frame_cb)
-			return;
+			return true;
 
 		fx->in_frame_cb = false;
 
 		if (done || fx->stale) {
 			fx->cb = NULL;
 			frame_xchg_done(fx, 0);
-			return;
+			return true;
 		}
 	}
 
-	return;
+	return false;
 
 early_frame:
 	/*
@@ -1018,13 +1032,21 @@ early_frame:
 	 * Save the response frame to be processed in the Tx done callback.
 	 */
 	if (fx->early_frame.mpdu)
-		return;
+		return false;
 
 	hdr_len = (const uint8_t *) body - (const uint8_t *) mpdu;
 	fx->early_frame.mpdu = l_memdup(mpdu, body_len + hdr_len);
 	fx->early_frame.body = (const uint8_t *) fx->early_frame.mpdu + hdr_len;
 	fx->early_frame.body_len = body_len;
 	fx->early_frame.rssi = rssi;
+	return false;
+}
+
+static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
+				const void *body, size_t body_len,
+				int rssi, void *user_data)
+{
+	frame_xchg_resp_handle(mpdu, body, body_len, rssi, user_data);
 }
 
 static bool frame_xchg_match(const void *a, const void *b)
@@ -1052,8 +1074,8 @@ static bool frame_xchg_match(const void *a, const void *b)
  */
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
-			uint32_t group_id, frame_xchg_cb_t cb, void *user_data,
-			va_list resp_args)
+			unsigned int retries_on_ack, uint32_t group_id,
+			frame_xchg_cb_t cb, void *user_data, va_list resp_args)
 {
 	struct frame_xchg_data *fx;
 	size_t frame_len;
@@ -1097,6 +1119,7 @@ void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 	fx->freq = freq;
 	fx->retry_interval = retry_interval;
 	fx->resp_timeout = resp_timeout;
+	fx->retries_on_ack = retries_on_ack;
 	fx->cb = cb;
 	fx->user_data = user_data;
 	fx->group_id = group_id;
diff --git a/src/frame-xchg.h b/src/frame-xchg.h
index 1855492c..93d528ae 100644
--- a/src/frame-xchg.h
+++ b/src/frame-xchg.h
@@ -45,6 +45,6 @@ bool frame_watch_wdev_remove(uint64_t wdev_id);
 
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
-			uint32_t group_id, frame_xchg_cb_t cb, void *user_data,
-			va_list resp_args);
+			unsigned int retries_on_ack, uint32_t group_id,
+			frame_xchg_cb_t cb, void *user_data, va_list resp_args);
 void frame_xchg_stop(uint64_t wdev_id);
-- 
2.20.1

[PATCH 11/12] frame-xchg: Add frame_xchg_start

[Andrew Zaborowski]

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

---
 src/frame-xchg.c | 13 +++++++++++++
 src/frame-xchg.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/src/frame-xchg.c b/src/frame-xchg.c
index a4b6b740..45113d6d 100644
--- a/src/frame-xchg.c
+++ b/src/frame-xchg.c
@@ -1072,6 +1072,19 @@ static bool frame_xchg_match(const void *a, const void *b)
  * @resp_timeout was 0.  @frame is an iovec array terminated by an iovec
  * struct with NULL-iov_base.
  */
+void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+			unsigned int retry_interval, unsigned int resp_timeout,
+			unsigned int retries_on_ack, uint32_t group_id,
+			frame_xchg_cb_t cb, void *user_data, ...)
+{
+	va_list args;
+
+	va_start(args, user_data);
+	frame_xchg_startv(wdev_id, frame, freq, retry_interval, resp_timeout,
+				retries_on_ack, group_id, cb, user_data, args);
+	va_end(args);
+}
+
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
diff --git a/src/frame-xchg.h b/src/frame-xchg.h
index 93d528ae..45b0e5fa 100644
--- a/src/frame-xchg.h
+++ b/src/frame-xchg.h
@@ -43,6 +43,10 @@ bool frame_watch_add(uint64_t wdev_id, uint32_t group, uint16_t frame_type,
 bool frame_watch_group_remove(uint64_t wdev_id, uint32_t group);
 bool frame_watch_wdev_remove(uint64_t wdev_id);
 
+void frame_xchg_start(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
+			unsigned int retry_interval, unsigned int resp_timeout,
+			unsigned int retries_on_ack, uint32_t group_id,
+			frame_xchg_cb_t cb, void *user_data, ...);
 void frame_xchg_startv(uint64_t wdev_id, struct iovec *frame, uint32_t freq,
 			unsigned int retry_interval, unsigned int resp_timeout,
 			unsigned int retries_on_ack, uint32_t group_id,
-- 
2.20.1

[PATCH 12/12] p2putil: Tolerate GO Neg Response with empty Channel List

[Andrew Zaborowski]

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

Work around a parse error in GO Negotiation with some P2P devices.
---
 src/p2putil.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/p2putil.c b/src/p2putil.c
index 02c02b4d..58a75a11 100644
--- a/src/p2putil.c
+++ b/src/p2putil.c
@@ -193,7 +193,12 @@ static bool extract_p2p_channel_list(const uint8_t *attr, size_t len,
 {
 	struct p2p_channel_list_attr *out = data;
 
-	if (len < 6)
+	/*
+	 * Some devices reply with an empty Channel Entry List inside the
+	 * Channel List attribute of a GO Negotiation Response (status 1),
+	 * so tolerate a length of 3.
+	 */
+	if (len < 3)
 		return false;
 
 	out->country[0] = *attr++;
-- 
2.20.1

Re: [PATCH 01/12] doc: Fix p2p method error names

[Denis Kenzior]

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

Hi Andrew,

On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> ---
>   doc/p2p-api.txt      | 2 +-
>   doc/p2p-peer-api.txt | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

Applied, thanks.

Regards,
-Denis

Re: [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback

[Denis Kenzior]

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

Hi Andrew,

On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> Don't try to handle l_io read errors in the read handler destroy
> callback.  That cb is called both when the read handler returns false,
> and when poll returns an error.  In neither case the l_io is
> automatically destroyed so we were wrongly setting group->io = NULL,
> we need to manually destroy the l_io.  But you can't use l_io_destroy
> safely from inside the read handler destroy callback.  Do this in the
> disconnect callback instead which is safe.
> ---
>   src/frame-xchg.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/frame-xchg.c b/src/frame-xchg.c
> index ce69c454..3719ed6c 100644
> --- a/src/frame-xchg.c
> +++ b/src/frame-xchg.c
> @@ -234,6 +234,7 @@ static void frame_watch_group_destroy(void *data)
>   		l_genl_remove_unicast_watch(iwd_get_genl(),
>   						group->unicast_watch_id);
>   
> +	l_io_set_disconnect_handler(group->io, NULL, NULL, NULL);

This part shouldn't be necessary?  io_closed clears out the 
disconnect_handler and disconnect_destroy callbacks...

Rest looks good.  Let me know if I should just edit out the change above.

Regards,
-Denis

Re: [PATCH 04/12] frame-xchg: Actually free duplicate watches

[Denis Kenzior]

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

Hi Andrew,

On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> Fix a potential leak when we need to drop an existing watch because it's
> being replaced with a new one.
> ---
>   src/frame-xchg.c | 1 +
>   1 file changed, 1 insertion(+)
> 

Patches 4-7 applied, thanks.

Regards,
-Denis

Re: [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 18 Mar 2020 at 16:49, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> > @@ -234,6 +234,7 @@ static void frame_watch_group_destroy(void *data)
> >               l_genl_remove_unicast_watch(iwd_get_genl(),
> >                                               group->unicast_watch_id);
> >
> > +     l_io_set_disconnect_handler(group->io, NULL, NULL, NULL);
>
> This part shouldn't be necessary?  io_closed clears out the
> disconnect_handler and disconnect_destroy callbacks...

This is meant to handle cases like l_queue_destroy(watch_groups,
frame_watch_group_destroy) where the l_io_destroy() may close the
socket, if not already closed, and call the disconnect handler which
would try to remove the group from watch_groups again, so we probably
do need this.

Best regards

Re: [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy

[Denis Kenzior]

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

Hi Andrew,

On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> If during WATCHLIST_NOTIFY{,_MATCHES,_NO_ARGS} one of the watch
> notify callback triggers a call to watchlist_destroy, give up calling
> remaining watches and destroy the watchlist without crashing.  This is
> useful in frame-xchg.c (P2P use case) where a frame watch may trigger
> a move to a new state after receiving a specific frame, and remove one
> group of frame watches (including its watchlist) to create a different
> group.
> ---
>   src/watchlist.c |  5 +++++
>   src/watchlist.h | 22 ++++++++++++++++++----
>   2 files changed, 23 insertions(+), 4 deletions(-)
> 

Patch 8, 9 and 12 applied, thanks.

Regards,
-Denis

Re: [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK

[Denis Kenzior]

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

Hi Andrew,

On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> In some cases a P2P peer will ACK our frame but not reply on the first
> attempt, and other implementations seem to handle this by going back to
> retransmitting the frame at a high rate until it gets ACKed again, at
> which point they will again give the peer a longer time to tx the
> response frame.  Implement the same logic here by adding a
> retries_on_ack parameter that takes the number of additional times we
> want to restart the normal retransmit counter after we received no
> response frame on the first attempt.  So passing 0 maintains the
> current behaviour, 1 for 1 extra attempt, etc.
> 
> In effect we may retransmit a frame about 15 * (retry_on_ack + 1) *
> <in-kernel retransmit limit> times.  The kernel/driver retransmits a
> frame a number of times if there's no ACK (I've seen about 20 normally)
> at a high frequency, if that fails we retry the whole process 15 times
> inside frame-xchg.c and if we still get no ACK at any point, we give up.
> If we do get an ACK, we wait for a response frame and if we don't get
> that we will optionally reset the retry counter and restart the whole
> thing retry_on_ack times.
> ---
>   src/frame-xchg.c | 61 +++++++++++++++++++++++++++++++++---------------
>   src/frame-xchg.h |  4 ++--
>   2 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/src/frame-xchg.c b/src/frame-xchg.c
> index a7565a24..a4b6b740 100644
> --- a/src/frame-xchg.c
> +++ b/src/frame-xchg.c
> @@ -107,6 +107,7 @@ struct frame_xchg_data {
>   	unsigned int retry_cnt;
>   	unsigned int retry_interval;
>   	unsigned int resp_timeout;
> +	unsigned int retries_on_ack;
>   	bool in_frame_cb : 1;
>   	bool stale : 1;
>   };
> @@ -722,12 +723,16 @@ static bool frame_watch_remove_by_handler(uint64_t wdev_id, uint32_t group_id,
>   	if (!group)
>   		return false;
>   
> +	l_error(" ** settign grp %i elems that match to nop", group->id);////

??

>   	return l_queue_foreach_remove(group->watches.items,
>   					frame_watch_item_remove_by_handler,
>   					&handler_info) > 0;
>   }
>   
>   static void frame_xchg_tx_retry(struct frame_xchg_data *fx);
> +static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
> +					const void *body, size_t body_len,
> +					int rssi, void *user_data);
>   static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
>   				const void *body, size_t body_len,
>   				int rssi, void *user_data);
> @@ -807,12 +812,19 @@ static void frame_xchg_timeout_cb(struct l_timeout *timeout,
>   	frame_xchg_tx_retry(fx);
>   }
>   
> -static void frame_xchg_resp_timeout_cb(struct l_timeout *timeout,
> +static void frame_xchg_listen_end_cb(struct l_timeout *timeout,
>   					void *user_data)
>   {
>   	struct frame_xchg_data *fx = user_data;
>   
> -	frame_xchg_done(fx, 0);
> +	if (!fx->retries_on_ack--) {

Is this post-decrement intended?  Might be better to do this in a way 
that doesn't require head-scratching for a minute :)

> +		frame_xchg_done(fx, 0);
> +		return;
> +	}
> +
> +	l_timeout_remove(fx->timeout);
> +	fx->retry_cnt = 0;
> +	frame_xchg_tx_retry(fx);
>   }
>   
>   static void frame_xchg_tx_status(struct frame_xchg_data *fx, bool acked)

Regards,
-Denis

Re: [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 18 Mar 2020 at 23:09, Denis Kenzior <denkenz@gmail.com> wrote:
> On 3/18/20 9:45 AM, Andrew Zaborowski wrote:
> > In some cases a P2P peer will ACK our frame but not reply on the first
> > attempt, and other implementations seem to handle this by going back to
> > retransmitting the frame at a high rate until it gets ACKed again, at
> > which point they will again give the peer a longer time to tx the
> > response frame.  Implement the same logic here by adding a
> > retries_on_ack parameter that takes the number of additional times we
> > want to restart the normal retransmit counter after we received no
> > response frame on the first attempt.  So passing 0 maintains the
> > current behaviour, 1 for 1 extra attempt, etc.
> >
> > In effect we may retransmit a frame about 15 * (retry_on_ack + 1) *
> > <in-kernel retransmit limit> times.  The kernel/driver retransmits a
> > frame a number of times if there's no ACK (I've seen about 20 normally)
> > at a high frequency, if that fails we retry the whole process 15 times
> > inside frame-xchg.c and if we still get no ACK at any point, we give up.
> > If we do get an ACK, we wait for a response frame and if we don't get
> > that we will optionally reset the retry counter and restart the whole
> > thing retry_on_ack times.
> > ---
> >   src/frame-xchg.c | 61 +++++++++++++++++++++++++++++++++---------------
> >   src/frame-xchg.h |  4 ++--
> >   2 files changed, 44 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/frame-xchg.c b/src/frame-xchg.c
> > index a7565a24..a4b6b740 100644
> > --- a/src/frame-xchg.c
> > +++ b/src/frame-xchg.c
> > @@ -107,6 +107,7 @@ struct frame_xchg_data {
> >       unsigned int retry_cnt;
> >       unsigned int retry_interval;
> >       unsigned int resp_timeout;
> > +     unsigned int retries_on_ack;
> >       bool in_frame_cb : 1;
> >       bool stale : 1;
> >   };
> > @@ -722,12 +723,16 @@ static bool frame_watch_remove_by_handler(uint64_t wdev_id, uint32_t group_id,
> >       if (!group)
> >               return false;
> >
> > +     l_error(" ** settign grp %i elems that match to nop", group->id);////
>
> ??

Oops, a git add screwup.
>
> >       return l_queue_foreach_remove(group->watches.items,
> >                                       frame_watch_item_remove_by_handler,
> >                                       &handler_info) > 0;
> >   }
> >
> >   static void frame_xchg_tx_retry(struct frame_xchg_data *fx);
> > +static bool frame_xchg_resp_handle(const struct mmpdu_header *mpdu,
> > +                                     const void *body, size_t body_len,
> > +                                     int rssi, void *user_data);
> >   static void frame_xchg_resp_cb(const struct mmpdu_header *mpdu,
> >                               const void *body, size_t body_len,
> >                               int rssi, void *user_data);
> > @@ -807,12 +812,19 @@ static void frame_xchg_timeout_cb(struct l_timeout *timeout,
> >       frame_xchg_tx_retry(fx);
> >   }
> >
> > -static void frame_xchg_resp_timeout_cb(struct l_timeout *timeout,
> > +static void frame_xchg_listen_end_cb(struct l_timeout *timeout,
> >                                       void *user_data)
> >   {
> >       struct frame_xchg_data *fx = user_data;
> >
> > -     frame_xchg_done(fx, 0);
> > +     if (!fx->retries_on_ack--) {
>
> Is this post-decrement intended?  Might be better to do this in a way
> that doesn't require head-scratching for a minute :)

Ok, but this is the same standard thing you use in a for loop ;)

Best regards