* [PATCH 02/12] cleanup: Remove extra empty lines
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback Andrew Zaborowski
` (10 subsequent siblings)
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 02/12] cleanup: Remove extra empty lines Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 15:50 ` Denis Kenzior
2020-03-18 14:45 ` [PATCH 04/12] frame-xchg: Actually free duplicate watches Andrew Zaborowski
` (9 subsequent siblings)
11 siblings, 1 reply; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback
2020-03-18 14:45 ` [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback Andrew Zaborowski
@ 2020-03-18 15:50 ` Denis Kenzior
2020-03-18 16:05 ` Andrew Zaborowski
0 siblings, 1 reply; 19+ messages in thread
From: Denis Kenzior @ 2020-03-18 15:50 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback
2020-03-18 15:50 ` Denis Kenzior
@ 2020-03-18 16:05 ` Andrew Zaborowski
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 16:05 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 04/12] frame-xchg: Actually free duplicate watches
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 02/12] cleanup: Remove extra empty lines Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 03/12] frame-xchg: Handle l_io errors through disconnect callback Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 15:53 ` Denis Kenzior
2020-03-18 14:45 ` [PATCH 05/12] frame-xchg: Fix frame_watch_item_remove_by_handler Andrew Zaborowski
` (8 subsequent siblings)
11 siblings, 1 reply; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 05/12] frame-xchg: Fix frame_watch_item_remove_by_handler
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (2 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 04/12] frame-xchg: Actually free duplicate watches Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 06/12] frame-xchg: Allow calling frame_xchg_stop from the callback Andrew Zaborowski
` (7 subsequent siblings)
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 06/12] frame-xchg: Allow calling frame_xchg_stop from the callback
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (3 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 05/12] frame-xchg: Fix frame_watch_item_remove_by_handler Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 07/12] frame-xchg: Allow frame_xchg_stop calls inside frame callbacks Andrew Zaborowski
` (6 subsequent siblings)
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 07/12] frame-xchg: Allow frame_xchg_stop calls inside frame callbacks
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (4 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 06/12] frame-xchg: Allow calling frame_xchg_stop from the callback Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy Andrew Zaborowski
` (5 subsequent siblings)
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (5 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 07/12] frame-xchg: Allow frame_xchg_stop calls inside frame callbacks Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 18:50 ` Denis Kenzior
2020-03-18 14:45 ` [PATCH 09/12] frame-xchg: Optimize frame_watch_remove_by_handler scenarios Andrew Zaborowski
` (4 subsequent siblings)
11 siblings, 1 reply; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy
2020-03-18 14:45 ` [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy Andrew Zaborowski
@ 2020-03-18 18:50 ` Denis Kenzior
0 siblings, 0 replies; 19+ messages in thread
From: Denis Kenzior @ 2020-03-18 18:50 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 09/12] frame-xchg: Optimize frame_watch_remove_by_handler scenarios
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (6 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 08/12] watchlist: Allow watch CBs to call watchlist_destroy Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK Andrew Zaborowski
` (3 subsequent siblings)
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (7 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 09/12] frame-xchg: Optimize frame_watch_remove_by_handler scenarios Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 18:53 ` Denis Kenzior
2020-03-18 14:45 ` [PATCH 11/12] frame-xchg: Add frame_xchg_start Andrew Zaborowski
` (2 subsequent siblings)
11 siblings, 1 reply; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK
2020-03-18 14:45 ` [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK Andrew Zaborowski
@ 2020-03-18 18:53 ` Denis Kenzior
2020-03-19 15:52 ` Andrew Zaborowski
0 siblings, 1 reply; 19+ messages in thread
From: Denis Kenzior @ 2020-03-18 18:53 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK
2020-03-18 18:53 ` Denis Kenzior
@ 2020-03-19 15:52 ` Andrew Zaborowski
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-19 15:52 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 11/12] frame-xchg: Add frame_xchg_start
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (8 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 10/12] frame-xchg: Add facility to keep retransmitting after ACK Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 14:45 ` [PATCH 12/12] p2putil: Tolerate GO Neg Response with empty Channel List Andrew Zaborowski
2020-03-18 15:21 ` [PATCH 01/12] doc: Fix p2p method error names Denis Kenzior
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 12/12] p2putil: Tolerate GO Neg Response with empty Channel List
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (9 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 11/12] frame-xchg: Add frame_xchg_start Andrew Zaborowski
@ 2020-03-18 14:45 ` Andrew Zaborowski
2020-03-18 15:21 ` [PATCH 01/12] doc: Fix p2p method error names Denis Kenzior
11 siblings, 0 replies; 19+ messages in thread
From: Andrew Zaborowski @ 2020-03-18 14:45 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 01/12] doc: Fix p2p method error names
2020-03-18 14:45 [PATCH 01/12] doc: Fix p2p method error names Andrew Zaborowski
` (10 preceding siblings ...)
2020-03-18 14:45 ` [PATCH 12/12] p2putil: Tolerate GO Neg Response with empty Channel List Andrew Zaborowski
@ 2020-03-18 15:21 ` Denis Kenzior
11 siblings, 0 replies; 19+ messages in thread
From: Denis Kenzior @ 2020-03-18 15:21 UTC (permalink / raw)
To: iwd
[-- 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
^ permalink raw reply [flat|nested] 19+ messages in thread