On Sat, Mar 09, 2013 at 07:40:48PM +0200, Mihail Costea wrote:
Subject: [RFC] batman-adv: Modified DAT structures and functions in order to support both IPv4 and IPv6
In order to support DHT for IPv6 there was needed a general implementation for the IP field in distributed-arp-table.c (now the IP is __be32 and this patch will transform it to unsigned char *). Distinction between types is done using a new enum.
Also all functions were changed to support this.
Signed-off-by: Mihail Costea mihail.costea90@gmail.com
Hi Mihail,
thanks for this patch. Sorry for the late reply, but I required some time to review it all :)
As first concern I would say that we need some more #ifdef (not thrown in the code, but placed in the right spots) to let the whole code work if somebody has disabled the IPv6 support in his kernel.
More comments inline
distributed-arp-table.c | 234 ++++++++++++++++++++++++++++++++++++----------- types.h | 21 ++++- 2 files changed, 197 insertions(+), 58 deletions(-)
diff --git a/distributed-arp-table.c b/distributed-arp-table.c index 9215caa..ed6e817 100644 --- a/distributed-arp-table.c +++ b/distributed-arp-table.c @@ -20,6 +20,7 @@ #include <linux/if_ether.h> #include <linux/if_arp.h> #include <net/arp.h> +#include <net/ipv6.h>
#include "main.h" #include "hash.h" @@ -31,6 +32,36 @@ #include "translation-table.h" #include "unicast.h"
+/* Prints similar debug message for IPv4 and IPv6.
- At list one variable parameter should be the IP itself, and it should
- be placed correctly based on format prefix and format suffix arguments.
- */
+#ifdef CONFIG_BATMAN_ADV_DEBUG
+#define batadv_dat_dbg_ip(bat_priv, ip_type, format_prefix, \
format_suffix, ...) \{ \switch (ip_type) { \case BATADV_DAT_IPv4: \batadv_dbg(BATADV_DBG_DAT, bat_priv, \format_prefix "%pI4" format_suffix, \__VA_ARGS__); \break; \case BATADV_DAT_IPv6: \batadv_dbg(BATADV_DBG_DAT, bat_priv, \format_prefix "%pI6c" format_suffix, \__VA_ARGS__); \break; \} \}+#else
+#define batadv_dat_dbg_ip(bat_priv, ip_type, format_prefix, \
format_suffix, ...) /* do nothing */
in this case we prefer to write a stub function rather than an empty define. In this way the type check can still work and report issues. See distributed-arp-table.h for an example.
+#endif /* CONFIG_BATMAN_ADV_DEBUG */
static void batadv_dat_purge(struct work_struct *work);
/** @@ -47,12 +78,14 @@ static void batadv_dat_start_timer(struct batadv_priv *bat_priv) /**
- batadv_dat_entry_free_ref - decrements the dat_entry refcounter and possibly
- free it
- @dat_entry: the oentry to free
*/
- @dat_entry: the entry to free
this is an example of change that should go in a separate patch as it is fixing a typo previously introduced.
static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry) {
- if (atomic_dec_and_test(&dat_entry->refcount))
- if (atomic_dec_and_test(&dat_entry->refcount)) {
kfree(dat_entry->ip);- }
mh, here we are mixing rcu freeing and not. it may be better to create a freeing function which take cares of freeing everything which will be set in this point by means of call_rcu() (you can grep for call_rcu in translation-table.c to see how we use it).
}
/** @@ -130,18 +163,50 @@ static void batadv_dat_purge(struct work_struct *work) }
/**
- batadv_sizeof_ip - gets sizeof IP based on its type (IPv4 / IPv6)
- @ip_type: type of IP address
- Returns sizeof IP, or sizeof IPv4 if @ip_type is invalid
no need to write the '@' here.
[...]
/**
- batadv_dat_types - types used in batadv_dat_entry for IP
- @BATADV_DAT_IPv4: IPv4 address type
- @BATADV_DAT_IPv6: IPv6 address type
- */
+enum batadv_dat_types {
- BATADV_DAT_IPv4,
- BATADV_DAT_IPv6,
+};
Constants should be all capital letters (look at the 'v')
+/**
- struct batadv_dat_candidate - candidate destination for DAT operations
- @type: the type of the selected candidate. It can one of the following:
- BATADV_DAT_CANDIDATE_NOT_FOUND--
What is not entirely clear to me (I may have overlooked it) is how do the two types of data interacts. If I am not wrong, you want to use the same table for both data, but this will create problems when comparing a 16bytes long ipv6 address with a 4bytes long ipv4 one. So you should assume that mixed compare can happen and you should check the type of the two objects before going to the real data compare.
I think you are on the right track, but you should think a bit more how to generalise your approach in order to account the case I described above.
It would also be nice to generalise it such that you do not have to talk about IPs, but about "DHT data" and its len, which can then be whatever we would like. However this point might be something we want to discuss later...now you can focus on your patch assuming we want to store IPv4/6 only.
Cheers,
p.s. feel free to join our IRC channel (#batman at irc.freenode.org) for live discussions)
Cheers²,