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: 5595 bytes --]

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);
>  		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).

>  }
> 
>  /**
> @@ -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²,

-- 
Antonio Quartulli

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

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