Re: [B.A.T.M.A.N.] [RFC] batman-adv: Modified DAT structures and functions in order to support both IPv4 and IPv6

[Antonio Quartulli <ordex@autistici.org>]

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

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,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]