[B.A.T.M.A.N.] [PATCH maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[B.A.T.M.A.N.] [PATCH maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[Matthias Schiffer]

Registering and unregistering the packet handlers on softif creation and
destruction is obviously broken when multiple softif are used (and causes the
second softif creation to fail). Instead, just set the network coding handlers
in the __init function (like all other handlers).

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 main.c           |  2 ++
 network-coding.c | 12 ++----------
 network-coding.h |  8 ++++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/main.c b/main.c
index 08125f3..87ef89a 100644
--- a/main.c
+++ b/main.c
@@ -349,6 +349,8 @@ static void batadv_recv_handler_init(void)
 	batadv_rx_handler[BATADV_TT_QUERY] = batadv_recv_tt_query;
 	/* Roaming advertisement */
 	batadv_rx_handler[BATADV_ROAM_ADV] = batadv_recv_roam_adv;
+	/* network coding */
+	batadv_rx_handler[BATADV_CODED] = batadv_nc_recv_coded_packet;
 }
 
 int
diff --git a/network-coding.c b/network-coding.c
index a487d46..6f392fd 100644
--- a/network-coding.c
+++ b/network-coding.c
@@ -31,8 +31,6 @@ static struct lock_class_key batadv_nc_coding_hash_lock_class_key;
 static struct lock_class_key batadv_nc_decoding_hash_lock_class_key;
 
 static void batadv_nc_worker(struct work_struct *work);
-static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
-				       struct batadv_hard_iface *recv_if);
 
 /**
  * batadv_nc_start_timer - initialise the nc periodic worker
@@ -70,11 +68,6 @@ int batadv_nc_init(struct batadv_priv *bat_priv)
 	batadv_hash_set_lock_class(bat_priv->nc.coding_hash,
 				   &batadv_nc_decoding_hash_lock_class_key);
 
-	/* Register our packet type */
-	if (batadv_recv_handler_register(BATADV_CODED,
-					 batadv_nc_recv_coded_packet) < 0)
-		goto err;
-
 	INIT_DELAYED_WORK(&bat_priv->nc.work, batadv_nc_worker);
 	batadv_nc_start_timer(bat_priv);
 
@@ -1657,8 +1650,8 @@ batadv_nc_find_decoding_packet(struct batadv_priv *bat_priv,
  * @skb: incoming coded packet
  * @recv_if: pointer to interface this packet was received on
  */
-static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
-				       struct batadv_hard_iface *recv_if)
+int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+				struct batadv_hard_iface *recv_if)
 {
 	struct batadv_priv *bat_priv = netdev_priv(recv_if->soft_iface);
 	struct batadv_unicast_packet *unicast_packet;
@@ -1726,7 +1719,6 @@ free_nc_packet:
  */
 void batadv_nc_free(struct batadv_priv *bat_priv)
 {
-	batadv_recv_handler_unregister(BATADV_CODED);
 	cancel_delayed_work_sync(&bat_priv->nc.work);
 
 	batadv_nc_purge_paths(bat_priv, bat_priv->nc.coding_hash, NULL);
diff --git a/network-coding.h b/network-coding.h
index 85a4ec8..f00cd4d 100644
--- a/network-coding.h
+++ b/network-coding.h
@@ -35,6 +35,8 @@ void batadv_nc_purge_orig(struct batadv_priv *bat_priv,
 					   struct batadv_nc_node *));
 void batadv_nc_init_bat_priv(struct batadv_priv *bat_priv);
 void batadv_nc_init_orig(struct batadv_orig_node *orig_node);
+int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+				struct batadv_hard_iface *recv_if);
 bool batadv_nc_skb_forward(struct sk_buff *skb,
 			   struct batadv_neigh_node *neigh_node);
 void batadv_nc_skb_store_for_decoding(struct batadv_priv *bat_priv,
@@ -85,6 +87,12 @@ static inline void batadv_nc_init_orig(struct batadv_orig_node *orig_node)
 	return;
 }
 
+static inline int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+					      struct batadv_hard_iface *recv_if)
+{
+	return NET_RX_DROP;
+}
+
 static inline bool batadv_nc_skb_forward(struct sk_buff *skb,
 					 struct batadv_neigh_node *neigh_node)
 {
-- 
1.8.4

[B.A.T.M.A.N.] [PATCHv2 maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[Matthias Schiffer]

batman-adv saves its table of packet handlers as a global state, so handlers
must be set up only once (and setting them up a second time will fail).

The recently-added network coding support tries to set up its handler each time
a new softif is registered, which obviously fails when more that one softif is
used (and in consequence, the softif creation fails).

Fix this by moving the handler setup to batadv_recv_handler_init(), which is
called by batman-adv's __init function (and where most other packet handlers are
set up).

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
v2: refined commit message

 main.c           |  2 ++
 network-coding.c | 12 ++----------
 network-coding.h |  8 ++++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/main.c b/main.c
index 08125f3..87ef89a 100644
--- a/main.c
+++ b/main.c
@@ -349,6 +349,8 @@ static void batadv_recv_handler_init(void)
 	batadv_rx_handler[BATADV_TT_QUERY] = batadv_recv_tt_query;
 	/* Roaming advertisement */
 	batadv_rx_handler[BATADV_ROAM_ADV] = batadv_recv_roam_adv;
+	/* network coding */
+	batadv_rx_handler[BATADV_CODED] = batadv_nc_recv_coded_packet;
 }
 
 int
diff --git a/network-coding.c b/network-coding.c
index a487d46..6f392fd 100644
--- a/network-coding.c
+++ b/network-coding.c
@@ -31,8 +31,6 @@ static struct lock_class_key batadv_nc_coding_hash_lock_class_key;
 static struct lock_class_key batadv_nc_decoding_hash_lock_class_key;
 
 static void batadv_nc_worker(struct work_struct *work);
-static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
-				       struct batadv_hard_iface *recv_if);
 
 /**
  * batadv_nc_start_timer - initialise the nc periodic worker
@@ -70,11 +68,6 @@ int batadv_nc_init(struct batadv_priv *bat_priv)
 	batadv_hash_set_lock_class(bat_priv->nc.coding_hash,
 				   &batadv_nc_decoding_hash_lock_class_key);
 
-	/* Register our packet type */
-	if (batadv_recv_handler_register(BATADV_CODED,
-					 batadv_nc_recv_coded_packet) < 0)
-		goto err;
-
 	INIT_DELAYED_WORK(&bat_priv->nc.work, batadv_nc_worker);
 	batadv_nc_start_timer(bat_priv);
 
@@ -1657,8 +1650,8 @@ batadv_nc_find_decoding_packet(struct batadv_priv *bat_priv,
  * @skb: incoming coded packet
  * @recv_if: pointer to interface this packet was received on
  */
-static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
-				       struct batadv_hard_iface *recv_if)
+int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+				struct batadv_hard_iface *recv_if)
 {
 	struct batadv_priv *bat_priv = netdev_priv(recv_if->soft_iface);
 	struct batadv_unicast_packet *unicast_packet;
@@ -1726,7 +1719,6 @@ free_nc_packet:
  */
 void batadv_nc_free(struct batadv_priv *bat_priv)
 {
-	batadv_recv_handler_unregister(BATADV_CODED);
 	cancel_delayed_work_sync(&bat_priv->nc.work);
 
 	batadv_nc_purge_paths(bat_priv, bat_priv->nc.coding_hash, NULL);
diff --git a/network-coding.h b/network-coding.h
index 85a4ec8..f00cd4d 100644
--- a/network-coding.h
+++ b/network-coding.h
@@ -35,6 +35,8 @@ void batadv_nc_purge_orig(struct batadv_priv *bat_priv,
 					   struct batadv_nc_node *));
 void batadv_nc_init_bat_priv(struct batadv_priv *bat_priv);
 void batadv_nc_init_orig(struct batadv_orig_node *orig_node);
+int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+				struct batadv_hard_iface *recv_if);
 bool batadv_nc_skb_forward(struct sk_buff *skb,
 			   struct batadv_neigh_node *neigh_node);
 void batadv_nc_skb_store_for_decoding(struct batadv_priv *bat_priv,
@@ -85,6 +87,12 @@ static inline void batadv_nc_init_orig(struct batadv_orig_node *orig_node)
 	return;
 }
 
+static inline int batadv_nc_recv_coded_packet(struct sk_buff *skb,
+					      struct batadv_hard_iface *recv_if)
+{
+	return NET_RX_DROP;
+}
+
 static inline bool batadv_nc_skb_forward(struct sk_buff *skb,
 					 struct batadv_neigh_node *neigh_node)
 {
-- 
1.8.4

Re: [B.A.T.M.A.N.] [PATCHv2 maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[Marek Lindner]

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

On Thursday 26 September 2013 10:29:49 Matthias Schiffer wrote:
> batman-adv saves its table of packet handlers as a global state, so handlers
> must be set up only once (and setting them up a second time will fail).
> 
> The recently-added network coding support tries to set up its handler each
> time a new softif is registered, which obviously fails when more that one
> softif is used (and in consequence, the softif creation fails).
> 
> Fix this by moving the handler setup to batadv_recv_handler_init(), which is
> called by batman-adv's __init function (and where most other packet
> handlers are set up).

Would you mind changing your approach to adding a global nc_init() / nc_free() 
and do the callback registration there ?

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv2 maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[Matthias Schiffer]

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

On 09/27/2013 08:07 AM, Marek Lindner wrote:
> On Thursday 26 September 2013 10:29:49 Matthias Schiffer wrote:
>> batman-adv saves its table of packet handlers as a global state, so handlers
>> must be set up only once (and setting them up a second time will fail).
>>
>> The recently-added network coding support tries to set up its handler each
>> time a new softif is registered, which obviously fails when more that one
>> softif is used (and in consequence, the softif creation fails).
>>
>> Fix this by moving the handler setup to batadv_recv_handler_init(), which is
>> called by batman-adv's __init function (and where most other packet
>> handlers are set up).
> 
> Would you mind changing your approach to adding a global nc_init() / nc_free() 
> and do the callback registration there ?
> 
> Thanks,
> Marek
> 

No problem. Any idea for a good name for the new function? Currently,
the init functions are named a bit inconsistently, there is
batadv_iv_init, which is called in batadv_init, and many other
batadv_*_init called in batadv_mesh_init. I'd prefer renaming all those
to batadv_*_mesh_init, but that seems like lots of avoidable changes for
a net/stable patch.

Maybe the maint patch could stay like it is, and I'll add another patch
on top for master that cleans things up?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [B.A.T.M.A.N.] [PATCHv2 maint] batman-adv: set network coding packet handlers in batadv_recv_handler_init()

[Marek Lindner]

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

On Friday 27 September 2013 10:13:51 Matthias Schiffer wrote:
> No problem. Any idea for a good name for the new function? Currently,
> the init functions are named a bit inconsistently, there is
> batadv_iv_init, which is called in batadv_init, and many other
> batadv_*_init called in batadv_mesh_init. I'd prefer renaming all those
> to batadv_*_mesh_init, but that seems like lots of avoidable changes for
> a net/stable patch.
> 
> Maybe the maint patch could stay like it is, and I'll add another patch
> on top for master that cleans things up?

Yes, you could come up with a naming convention you introduce with the maint 
patch for nc_init() / nc_free().  Other renamings could go into next or 
master.

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[B.A.T.M.A.N.] [PATCHv3 maint] batman-adv: set up network coding packet handlers during module init

[Matthias Schiffer]

batman-adv saves its table of packet handlers as a global state, so handlers
must be set up only once (and setting them up a second time will fail).

The recently-added network coding support tries to set up its handler each time
a new softif is registered, which obviously fails when more that one softif is
used (and in consequence, the softif creation fails).

Fix this by splitting up batadv_nc_init into batadv_nc_init (which is called
only once) and batadv_nc_mesh_init (which is called for each softif); in
addition batadv_nc_free is renamed to batadv_nc_mesh_free to keep naming
consistent.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
v3: moved things around as Marek suggested

 main.c           |  5 +++--
 network-coding.c | 28 ++++++++++++++++++----------
 network-coding.h | 14 ++++++++++----
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/main.c b/main.c
index 08125f3..c8e0671 100644
--- a/main.c
+++ b/main.c
@@ -61,6 +61,7 @@ static int __init batadv_init(void)
 	batadv_recv_handler_init();
 
 	batadv_iv_init();
+	batadv_nc_init();
 
 	batadv_event_workqueue = create_singlethread_workqueue("bat_events");
 
@@ -138,7 +139,7 @@ int batadv_mesh_init(struct net_device *soft_iface)
 	if (ret < 0)
 		goto err;
 
-	ret = batadv_nc_init(bat_priv);
+	ret = batadv_nc_mesh_init(bat_priv);
 	if (ret < 0)
 		goto err;
 
@@ -163,7 +164,7 @@ void batadv_mesh_free(struct net_device *soft_iface)
 	batadv_vis_quit(bat_priv);
 
 	batadv_gw_node_purge(bat_priv);
-	batadv_nc_free(bat_priv);
+	batadv_nc_mesh_free(bat_priv);
 	batadv_dat_free(bat_priv);
 	batadv_bla_free(bat_priv);
 
diff --git a/network-coding.c b/network-coding.c
index a487d46..4ecc0b6 100644
--- a/network-coding.c
+++ b/network-coding.c
@@ -35,6 +35,20 @@ static int batadv_nc_recv_coded_packet(struct sk_buff *skb,
 				       struct batadv_hard_iface *recv_if);
 
 /**
+ * batadv_nc_init - one-time initialization for network coding
+ */
+int __init batadv_nc_init(void)
+{
+	int ret;
+
+	/* Register our packet type */
+	ret = batadv_recv_handler_register(BATADV_CODED,
+					   batadv_nc_recv_coded_packet);
+
+	return ret;
+}
+
+/**
  * batadv_nc_start_timer - initialise the nc periodic worker
  * @bat_priv: the bat priv with all the soft interface information
  */
@@ -45,10 +59,10 @@ static void batadv_nc_start_timer(struct batadv_priv *bat_priv)
 }
 
 /**
- * batadv_nc_init - initialise coding hash table and start house keeping
+ * batadv_nc_mesh_init - initialise coding hash table and start house keeping
  * @bat_priv: the bat priv with all the soft interface information
  */
-int batadv_nc_init(struct batadv_priv *bat_priv)
+int batadv_nc_mesh_init(struct batadv_priv *bat_priv)
 {
 	bat_priv->nc.timestamp_fwd_flush = jiffies;
 	bat_priv->nc.timestamp_sniffed_purge = jiffies;
@@ -70,11 +84,6 @@ int batadv_nc_init(struct batadv_priv *bat_priv)
 	batadv_hash_set_lock_class(bat_priv->nc.coding_hash,
 				   &batadv_nc_decoding_hash_lock_class_key);
 
-	/* Register our packet type */
-	if (batadv_recv_handler_register(BATADV_CODED,
-					 batadv_nc_recv_coded_packet) < 0)
-		goto err;
-
 	INIT_DELAYED_WORK(&bat_priv->nc.work, batadv_nc_worker);
 	batadv_nc_start_timer(bat_priv);
 
@@ -1721,12 +1730,11 @@ free_nc_packet:
 }
 
 /**
- * batadv_nc_free - clean up network coding memory
+ * batadv_nc_mesh_free - clean up network coding memory
  * @bat_priv: the bat priv with all the soft interface information
  */
-void batadv_nc_free(struct batadv_priv *bat_priv)
+void batadv_nc_mesh_free(struct batadv_priv *bat_priv)
 {
-	batadv_recv_handler_unregister(BATADV_CODED);
 	cancel_delayed_work_sync(&bat_priv->nc.work);
 
 	batadv_nc_purge_paths(bat_priv, bat_priv->nc.coding_hash, NULL);
diff --git a/network-coding.h b/network-coding.h
index 85a4ec8..ddfa618 100644
--- a/network-coding.h
+++ b/network-coding.h
@@ -22,8 +22,9 @@
 
 #ifdef CONFIG_BATMAN_ADV_NC
 
-int batadv_nc_init(struct batadv_priv *bat_priv);
-void batadv_nc_free(struct batadv_priv *bat_priv);
+int batadv_nc_init(void);
+int batadv_nc_mesh_init(struct batadv_priv *bat_priv);
+void batadv_nc_mesh_free(struct batadv_priv *bat_priv);
 void batadv_nc_update_nc_node(struct batadv_priv *bat_priv,
 			      struct batadv_orig_node *orig_node,
 			      struct batadv_orig_node *orig_neigh_node,
@@ -46,12 +47,17 @@ int batadv_nc_init_debugfs(struct batadv_priv *bat_priv);
 
 #else /* ifdef CONFIG_BATMAN_ADV_NC */
 
-static inline int batadv_nc_init(struct batadv_priv *bat_priv)
+static inline int batadv_nc_init(void)
 {
 	return 0;
 }
 
-static inline void batadv_nc_free(struct batadv_priv *bat_priv)
+static inline int batadv_nc_mesh_init(struct batadv_priv *bat_priv)
+{
+	return 0;
+}
+
+static inline void batadv_nc_mesh_free(struct batadv_priv *bat_priv)
 {
 	return;
 }
-- 
1.8.4

Re: [B.A.T.M.A.N.] [PATCHv3 maint] batman-adv: set up network coding packet handlers during module init

[Marek Lindner]

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

On Friday 27 September 2013 18:03:39 Matthias Schiffer wrote:
> batman-adv saves its table of packet handlers as a global state, so handlers
> must be set up only once (and setting them up a second time will fail).
> 
> The recently-added network coding support tries to set up its handler each
> time a new softif is registered, which obviously fails when more that one
> softif is used (and in consequence, the softif creation fails).
> 
> Fix this by splitting up batadv_nc_init into batadv_nc_init (which is called
> only once) and batadv_nc_mesh_init (which is called for each softif); in
> addition batadv_nc_free is renamed to batadv_nc_mesh_free to keep naming
> consistent.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
> v3: moved things around as Marek suggested
> 
>  main.c           |  5 +++--
>  network-coding.c | 28 ++++++++++++++++++----------
>  network-coding.h | 14 ++++++++++----
>  3 files changed, 31 insertions(+), 16 deletions(-)

Applied in revision 586cf84.

Thanks,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]