On Fri, Mar 15, 2013 at 03:19:22PM +0200, Mihail Costea wrote:
On 15 March 2013 13:57, Antonio Quartulli ordex@autistici.org wrote:
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.
Thx for the info. I haven't thought about this. I'll think how it's best to work with the IPv6 support.
ok
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.
Should both defines be functions or only the empty define? It's not a big problem. It would be nice to play more with functions with variable args too :).
I'd say to use functions for both.
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); kfree_rcu(dat_entry, rcu);}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).
Thx. I saw that if we use call_rcu() there's no need to use kfree_rcu(), but only kfree().
correct, but in the function invoked by call_rcu, not here.
+/**
- 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.
About this I think we could use container_of or something similiar maybe to get the whole dat_entry structure and then test if types are equal.
yeah, should be good. Unless we can pass the entire dat_entry to the compare function, but I think this is not really doable (haven't looked in it now).
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.
Good to know. I would prefer for this patch to keep it similar so we wouldn't lose time with understanding the patch again.
sure
Cheers,
p.s. feel free to join our IRC channel (#batman at irc.freenode.org) for live discussions)
Cheers²,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Thx a lot for the review. Monday I will send the update to the changes. Should it be in this thread or a new thread?
better a new thread and postpone "v2" to the RFC word. In this way everybody know that you are posting a new version of something that went over the ml in the past.
If possible, after the "---" (it you open the .patch file with an editor you will notice the --- right after the message body) it would be good to have a short summary of what you changed since v1. In this way, whatever the version of your RFC/PATCH everybody can easily understand what you changed since your last post.
Cheers,