Patchwork l2_packet: fix eloop leak when using l2_packet_none

login
register
mail settings
Submitter Maxime Bizon
Date March 20, 2014, 6:24 p.m.
Message ID <1395339893-4561-1-git-send-email-mbizon@freebox.fr>
Download mbox | patch
Permalink /patch/332349/
State Superseded
Headers show

Comments

Maxime Bizon - March 20, 2014, 6:24 p.m.
Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
---
 src/l2_packet/l2_packet_none.c |    2 --
 1 file changed, 2 deletions(-)
Jouni Malinen - April 13, 2014, 9:20 p.m.
On Thu, Mar 20, 2014 at 07:24:53PM +0100, Maxime Bizon wrote:
>  src/l2_packet/l2_packet_none.c |    2 --

What are you using l2_packet_none.c for? It is just a minimal template
showing how an l2_packet wrapper could be implemented.

> diff --git a/src/l2_packet/l2_packet_none.c b/src/l2_packet/l2_packet_none.c
> @@ -84,7 +84,6 @@ struct l2_packet_data * l2_packet_init(
> -	eloop_register_read_sock(l2->fd, l2_packet_receive, l2, NULL);

This would remove the only use of l2_packet_receive() and result in a
compilation warning (or error in my case since I include -Werror for
gcc).

> @@ -96,7 +95,6 @@ void l2_packet_deinit(struct l2_packet_data *l2)
>  	if (l2->fd >= 0) {
> -		eloop_unregister_read_sock(l2->fd);
>  		/* TODO: close connection */

Would probably be more valuable for the example to maintain these calls
in the file somehow, e.g., by making registration use something like "if
(l2->fd >= 0)" as the condition (i.e., compiler will remove it, but does
not complain about unused static function).
Maxime Bizon - April 14, 2014, 11:48 a.m.
On Mon, 2014-04-14 at 00:20 +0300, Jouni Malinen wrote:

> What are you using l2_packet_none.c for? It is just a minimal template
> showing how an l2_packet wrapper could be implemented.

if you don't have CONFIG_L2_PACKET=y, it happens to use l2_packet_none
implementation automatically

I don't have CONFIG_RSN_PREAUTH so it is not selected automatically

the code looked more to me like a "do nothing" layer instead of a
reference implementation

> Would probably be more valuable for the example to maintain these calls
> in the file somehow, e.g., by making registration use something like "if
> (l2->fd >= 0)" as the condition (i.e., compiler will remove it, but does
> not complain about unused static function).

I've not tested this, but since my eloop patch was applied (assert(fd !=
-1)), usage of l2_packet_none should not be possible anymore.

maybe we should leave l2_packet_none as-is or rename it and use empty
static inline functions when we don't need it ?
Jouni Malinen - April 14, 2014, 8:09 p.m.
On Mon, Apr 14, 2014 at 01:48:42PM +0200, Maxime Bizon wrote:
> if you don't have CONFIG_L2_PACKET=y, it happens to use l2_packet_none
> implementation automatically
> 
> I don't have CONFIG_RSN_PREAUTH so it is not selected automatically
> 
> the code looked more to me like a "do nothing" layer instead of a
> reference implementation

Ah, I see. Somehow I did not even remember this being a possibility
anymore since l2_packet is used in number of places and wpa_supplicant
uses l2_packet_linux.c by default. Anyway, I see now that this is not
the case for hostapd.

> I've not tested this, but since my eloop patch was applied (assert(fd !=
> -1)), usage of l2_packet_none should not be possible anymore.
> 
> maybe we should leave l2_packet_none as-is or rename it and use empty
> static inline functions when we don't need it ?

It's fine to just skip the eloop_register_read_sock() call while leaving
the example there. I'll fix this.

Patch

diff --git a/src/l2_packet/l2_packet_none.c b/src/l2_packet/l2_packet_none.c
index b01e830..2663768 100644
--- a/src/l2_packet/l2_packet_none.c
+++ b/src/l2_packet/l2_packet_none.c
@@ -84,7 +84,6 @@  struct l2_packet_data * l2_packet_init(
 	 * TODO: open connection for receiving frames
 	 */
 	l2->fd = -1;
-	eloop_register_read_sock(l2->fd, l2_packet_receive, l2, NULL);
 
 	return l2;
 }
@@ -96,7 +95,6 @@  void l2_packet_deinit(struct l2_packet_data *l2)
 		return;
 
 	if (l2->fd >= 0) {
-		eloop_unregister_read_sock(l2->fd);
 		/* TODO: close connection */
 	}