Re: [B.A.T.M.A.N.] [RFC 2/4] batman-adv: added IPv6 to DAT and generic functions in distributed-arp-table.c

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, May 29, 2013 at 08:21:37AM -0700, Mihail Costea wrote:
> On 26 May 2013 06:57, Antonio Quartulli <ordex@autistici.org> wrote:
> > On Fri, May 17, 2013 at 03:27:50PM +0300, Mihail wrote:
> >> From: "mihail.costea90@gmail.com" <mihail.costea90@gmail.com>
> >>
> >> Added IPv6 functionality to generic functions implemented in the first
> >> patch.
> >
> > avoid references to "first"/"second"/"third" patch. Once committed, they will
> > just be some patches in the list. You can either explicitly mention the name of
> > the patch ("writing the subject in this way") or just saying
> > previously/whatever else.
> >
> >
> 
> ACK
> 
> >>
> >> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
> >> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
> >> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
> >>
> >> ---
> >>  distributed-arp-table.c |   30 ++++++++++++++++++++++++++----
> >>  types.h                 |    6 +++++-
> >>  2 files changed, 31 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> >> index b2ca7e0..42118be 100644
> >> --- a/distributed-arp-table.c
> >> +++ b/distributed-arp-table.c
> >> @@ -32,7 +32,7 @@
> >>  #include "unicast.h"
> >>
> >>  static char *batadv_dat_types_str_fmt[] = {
> >> -     "%pI4",
> >> +     "%pI4", "%pI6c",
> >
> > better going on a new line rather than putting both on the same one (like the
> > array declared in the gateway code).
> >
> 
> ACK
> 
> >>  };
> >>
> >>  static void batadv_dat_purge(struct work_struct *work);
> >> @@ -173,6 +173,10 @@ static size_t batadv_sizeof_dat_data(uint8_t data_type)
> >>       switch (data_type) {
> >>       case BATADV_DAT_IPV4:
> >>               return sizeof(__be32);
> >> +#if IS_ENABLED(CONFIG_IPV6)
> >> +     case BATADV_DAT_IPV6:
> >> +             return sizeof(struct in6_addr);
> >> +#endif
> >>       default:
> >>               return 0;
> >>       }
> >> @@ -295,6 +299,12 @@ static uint32_t batadv_hash_dat_ipv4(const void *data, uint32_t size)
> >>       return batadv_hash_dat(data, BATADV_DAT_IPV4, size);
> >>  }
> >>
> >> +#if IS_ENABLED(CONFIG_IPV6)
> >> +static uint32_t batadv_hash_dat_ipv6(const void *data, uint32_t size)
> >> +{
> >> +     return batadv_hash_dat(data, BATADV_DAT_IPV6, size);
> >> +}
> >> +#endif
> >>
> >>  /**
> >>   * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
> >> @@ -393,6 +403,11 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data,
> >>       case BATADV_DAT_IPV4:
> >>               choose = batadv_hash_dat_ipv4;
> >>               break;
> >> +#if IS_ENABLED(CONFIG_IPV6)
> >> +     case BATADV_DAT_IPV6:
> >> +             choose = batadv_hash_dat_ipv6;
> >> +             break;
> >> +#endif
> >
> > mh...I was just wondering: for the hash functions it would be nice to re-use the
> > same "array approach" that you used for the printing format.
> >
> > So we would have an array of function pointers to reference rather than having
> > this switch block...What do you think?
> >
> > I think it would be possible to do this for many other "variable" parameters if
> > required.
> >
> 
> Should there be a struct that contains all of them, like:
> 
> struct batadv_dat_data  {
>   char *format;
>   signature *hash_function;
>   .......
> }
> 
> And after that the array with the initialization for all types.
> Or should they be declared as arrays that have nothing in common? Like
> just adding a new array for the choose functions.
> 

I guess you replied to the same email twice.
However, the idea of 1  array of structs is nice! I like it :)


-- 
Antonio Quartulli

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

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