[RFCv3] batman-adv: Modified DAT structures and functions in order to support both IPv4 and IPv6

  Subject: [RFCv3] batman-adv: Modified DAT structures and functions in order
 to support both IPv4 and IPv6

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>

---

Removed the ugly debug function with 2 macros (one is to test if IPV6 is
enabled). I'm open the better ideas (the debug macro is used in 4 places
to reduce switch-cases).
Changed "if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)" to
"if IS_ENABLED(CONFIG_IPV6)".
Changed batadv_dat_entry ip member from unsigned char * to void * and renamed
it to data.
Use only one batadv_compare_dat function.

 distributed-arp-table.c |  255 +++++++++++++++++++++++++++++++++++++----------
 types.h                 |   24 ++++-
 2 files changed, 224 insertions(+), 55 deletions(-)

  struct hlist_node hash_entry;
@@ -980,6 +982,18 @@ struct batadv_dat_entry {
 };

 /**
+ * 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,
+#if IS_ENABLED(CONFIG_IPV6)
+ BATADV_DAT_IPV6,
+#endif
+};
+
+/**
  * 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
--
1.7.10.4
  

Hi Mihail,

On Mon, Apr 15, 2013 at 12:37:33PM +0300, Mihail Costea wrote:

[...]

> 
> +#ifdef CONFIG_BATMAN_ADV_DEBUG
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +
> +/* batadv_dat_dbg_ip - print similar debug message for IPv4 and IPv6.
> + * @bat_priv: the bat priv with all the soft interface information
> + * @ip_type: IPv4 / IPv6 address
> + * @format_prefix: format before IP field
> + * @format_suffix: format after IP field
> + *
> + * At list one variable parameter should be the IP itself, and it should
> + * be placed correctly based on format prefix and format suffix arguments.
> + */
> +#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; \
> + } \
> + }

I think this define is pretty overkill..What about creating a function which
only takes care of converting a generic DHT data in a string. Later you can
invoke this function when you want to print an IPv4/6 using batadv_dbg.

Here is an example:

char *batadv_dat_data_to_str(struct batadv_dat_entry *dat_entry,
			     const char *buf, size_t buf_len)
{
	/* do something and put the string representation of the entry in the
	 * buf, without exceeding buf_len.
	 * Remember to use IS_ENABLED(CONFIG_IPV6) inside
	 */


	return buf;
}

Then you can call it directly as parameter of a batdv_dbg function. E.g:

batadv_dbg(....," %s\n", batadv_dat_data_to_str(entry, buf, buf_len));

I think this approach would be better and easy to extend for future data types.
If you like this approach, what do you think about sending a patch now to add
this function to the current code?

> +
>  static void batadv_dat_purge(struct work_struct *work);
> 
>  /**
> @@ -45,6 +101,19 @@ static void batadv_dat_start_timer(struct
> batadv_priv *bat_priv)
>  }
> 
>  /**
> + * batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
> + * @rcu: the dat entry rcu
> + */
> +static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu)
> +{
> + struct batadv_dat_entry *dat_entry;
> +
> + dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
> + kfree(dat_entry->data);
> + kfree(dat_entry);
> +}
> +
> +/**
>   * batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
>   * free it
>   * @dat_entry: the entry to free
> @@ -52,7 +121,7 @@ static void batadv_dat_start_timer(struct
> batadv_priv *bat_priv)
>  static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry)
>  {
>   if (atomic_dec_and_test(&dat_entry->refcount))
> - kfree_rcu(dat_entry, rcu);
> + call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);
>  }
> 
>  /**
> @@ -130,6 +199,26 @@ static void batadv_dat_purge(struct work_struct *work)
>  }
> 
>  /**
> + * batadv_sizeof_ip - get 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.
> + */
> +static size_t batadv_sizeof_ip(uint8_t ip_type)
> +{
> + switch (ip_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 sizeof(__be32); /* fallback to IPv4 */
> + }
> +}

Either you mail client killed the tabs or this switch is not indented properly.
Also, do you think it would be better to return 0 in case of unknown type? In
this way we prevent any further operation on the data buffer.

> +
> +/**
>   * batadv_compare_dat - comparing function used in the local DAT hash table
>   * @node: node in the local table
>   * @data2: second object to compare the node to
> @@ -138,10 +227,18 @@ static void batadv_dat_purge(struct work_struct *work)
>   */
>  static int batadv_compare_dat(const struct hlist_node *node, const void *data2)
>  {
> - const void *data1 = container_of(node, struct batadv_dat_entry,
> - hash_entry);
> + struct batadv_dat_entry *dat_entry1 = container_of(node,
> + struct batadv_dat_entry, hash_entry);
> + struct batadv_dat_entry *dat_entry2 = container_of(data2,
> + struct batadv_dat_entry, data);
> + size_t ip_size;
> 
> - return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
> + if (dat_entry1->type != dat_entry2->type)
> + return 0;
> +
> + ip_size = batadv_sizeof_ip(dat_entry1->type);
> + return (memcmp(dat_entry1->data, dat_entry2->data,
> +       ip_size) == 0 ? 1 : 0);
>  }
> 
>  /**
> @@ -201,16 +298,19 @@ static __be32 batadv_arp_ip_dst(struct sk_buff
> *skb, int hdr_size)
>   * batadv_hash_dat - compute the hash value for an IP address
>   * @data: data to hash
>   * @size: size of the hash table
> + * @ip_type: type of IP address
>   *
>   * Returns the selected index in the hash table for the given data.
>   */
> -static uint32_t batadv_hash_dat(const void *data, uint32_t size)
> +static uint32_t batadv_hash_dat(const void *data, uint32_t size,
> +       uint8_t ip_type)
>  {
>   const unsigned char *key = data;
>   uint32_t hash = 0;
> - size_t i;
> + size_t i, ip_size;
> 
> - for (i = 0; i < 4; i++) {
> + ip_size = batadv_sizeof_ip(ip_type);
> + for (i = 0; i < ip_size; i++) {
>   hash += key[i];
>   hash += (hash << 10);
>   hash ^= (hash >> 6);
> @@ -223,31 +323,47 @@ static uint32_t batadv_hash_dat(const void
> *data, uint32_t size)
>   return hash % size;
>  }

Also here, I think it is a mail client problem at this point :)
Did you send the patch using git send-email? It will take care of not ruining
the text ;)

> 
> +static uint32_t batadv_hash_dat_ipv4(const void *data, uint32_t size)
> +{
> + return batadv_hash_dat(data, size, BATADV_DAT_IPV4);
> +}
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> +static uint32_t batadv_hash_dat_ipv6(const void *data, uint32_t size)
> +{
> + return batadv_hash_dat(data, size, BATADV_DAT_IPV6);
> +}
> +#endif
> +
>  /**
>   * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
>   * table
>   * @bat_priv: the bat priv with all the soft interface information
>   * @ip: search key
> + * @ip_type: type of IP address
>   *
>   * Returns the dat_entry if found, NULL otherwise.
>   */
>  static struct batadv_dat_entry *
> -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip)
> +batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *ip,
> +   uint8_t ip_type)
>  {
>   struct hlist_head *head;
>   struct batadv_dat_entry *dat_entry, *dat_entry_tmp = NULL;
>   struct batadv_hashtable *hash = bat_priv->dat.hash;
>   uint32_t index;
> + size_t ip_size;
> 
>   if (!hash)
>   return NULL;
> 
> - index = batadv_hash_dat(&ip, hash->size);
> + ip_size = batadv_sizeof_ip(ip_type);
> + index = batadv_hash_dat(ip, hash->size, ip_type);
>   head = &hash->table[index];
> 
>   rcu_read_lock();
>   hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
> - if (dat_entry->ip != ip)
> + if (memcmp(dat_entry->data, ip, ip_size))
>   continue;
> 

Why don't you check the type first? if dat_entry->type != ip_type then you can
"continue;" and skip the memory check. If you don't do so you may do a memory
check beyond the limit of dat_entry->data if it is smaller than ip_size.

>   if (!atomic_inc_not_zero(&dat_entry->refcount))
> @@ -264,24 +380,28 @@ batadv_dat_entry_hash_find(struct batadv_priv
> *bat_priv, __be32 ip)
>  /**
>   * batadv_dat_entry_add - add a new dat entry or update it if already exists
>   * @bat_priv: the bat priv with all the soft interface information
> - * @ip: ipv4 to add/edit
> + * @ip: IP to add/edit
> + * @ip_type: type of IP address
>   * @mac_addr: mac address to assign to the given ipv4
>   */
> -static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
> - uint8_t *mac_addr)
> +static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *ip,
> + uint8_t ip_type, uint8_t *mac_addr)
>  {
>   struct batadv_dat_entry *dat_entry;
>   int hash_added;
> + size_t ip_size;
> + batadv_hashdata_choose_cb choose;
> 
> - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip);
> + dat_entry = batadv_dat_entry_hash_find(bat_priv, ip, ip_type);
>   /* if this entry is already known, just update it */
>   if (dat_entry) {
>   if (!batadv_compare_eth(dat_entry->mac_addr, mac_addr))
>   memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN);
>   dat_entry->last_update = jiffies;
> - batadv_dbg(BATADV_DBG_DAT, bat_priv,
> -   "Entry updated: %pI4 %pM\n", &dat_entry->ip,
> -   dat_entry->mac_addr);
> +
> + batadv_dat_dbg_ip(bat_priv, ip_type, "Entry updated: ",
> +  " %pM\n", dat_entry->data,
> +  dat_entry->mac_addr);
>   goto out;
>   }
> 
> @@ -289,13 +409,30 @@ static void batadv_dat_entry_add(struct
> batadv_priv *bat_priv, __be32 ip,
>   if (!dat_entry)
>   goto out;
> 
> - dat_entry->ip = ip;
> + ip_size = batadv_sizeof_ip(ip_type);
> + dat_entry->data = kmalloc(ip_size, GFP_ATOMIC);
> + if (!dat_entry->data)
> + goto out;
> + memcpy(dat_entry->data, ip, ip_size);
> + dat_entry->type = ip_type;
> +
>   memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN);
>   dat_entry->last_update = jiffies;
>   atomic_set(&dat_entry->refcount, 2);
> 
> + switch (ip_type) {
> +#if IS_ENABLED(CONFIG_IPV6)
> + case BATADV_DAT_IPV6:
> + choose = batadv_hash_dat_ipv6;
> + break;
> +#endif
> + default:
> + choose = batadv_hash_dat_ipv4; /* IPv4 by default */
> + break;
> + }
> +
>   hash_added = batadv_hash_add(bat_priv->dat.hash, batadv_compare_dat,
> -     batadv_hash_dat, &dat_entry->ip,
> +     choose, dat_entry->data,
>       &dat_entry->hash_entry);
> 
>   if (unlikely(hash_added != 0)) {
> @@ -304,9 +441,8 @@ static void batadv_dat_entry_add(struct
> batadv_priv *bat_priv, __be32 ip,
>   goto out;
>   }
> 
> - batadv_dbg(BATADV_DBG_DAT, bat_priv, "New entry added: %pI4 %pM\n",
> -   &dat_entry->ip, dat_entry->mac_addr);
> -
> + batadv_dat_dbg_ip(bat_priv, ip_type, "New entry added: ", " %pM\n",
> +  dat_entry->data, dat_entry->mac_addr);

Here you can use the "new function" I told you about at the beginning of the
email.

>  out:
>   if (dat_entry)
>   batadv_dat_entry_free_ref(dat_entry);
> @@ -513,7 +649,8 @@ static void batadv_choose_next_candidate(struct
> batadv_priv *bat_priv,
>   * batadv_dat_select_candidates - select the nodes which the DHT message has to
>   * be sent to
>   * @bat_priv: the bat priv with all the soft interface information
> - * @ip_dst: ipv4 to look up in the DHT
> + * @ip_dst: IP to look up in the DHT
> + * @ip_type: type of IP address
>   *
>   * An originator O is selected if and only if its DHT_ID value is one of three
>   * closest values (from the LEFT, with wrap around if needed) then the hash
> @@ -522,7 +659,8 @@ static void batadv_choose_next_candidate(struct
> batadv_priv *bat_priv,
>   * Returns the candidate array of size BATADV_DAT_CANDIDATE_NUM.
>   */
>  static struct batadv_dat_candidate *
> -batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst)
> +batadv_dat_select_candidates(struct batadv_priv *bat_priv, void *ip_dst,
> +     uint8_t ip_type)
>  {
>   int select;
>   batadv_dat_addr_t last_max = BATADV_DAT_ADDR_MAX, ip_key;
> @@ -535,12 +673,11 @@ batadv_dat_select_candidates(struct batadv_priv
> *bat_priv, __be32 ip_dst)
>   if (!res)
>   return NULL;
> 
> - ip_key = (batadv_dat_addr_t)batadv_hash_dat(&ip_dst,
> -    BATADV_DAT_ADDR_MAX);
> + ip_key = (batadv_dat_addr_t)batadv_hash_dat(ip_dst, BATADV_DAT_ADDR_MAX,
> + ip_type);
> 
> - batadv_dbg(BATADV_DBG_DAT, bat_priv,
> -   "dat_select_candidates(): IP=%pI4 hash(IP)=%u\n", &ip_dst,
> -   ip_key);
> + batadv_dat_dbg_ip(bat_priv, ip_type, "dat_select_candidates(): IP=",
> +  " hash(IP)=%u\n", ip_dst, ip_key);
> 
>   for (select = 0; select < BATADV_DAT_CANDIDATES_NUM; select++)
>   batadv_choose_next_candidate(bat_priv, res, select, ip_key,
> @@ -554,6 +691,7 @@ batadv_dat_select_candidates(struct batadv_priv
> *bat_priv, __be32 ip_dst)
>   * @bat_priv: the bat priv with all the soft interface information
>   * @skb: payload to send
>   * @ip: the DHT key
> + * @ip_type: type of IP address
>   * @packet_subtype: unicast4addr packet subtype to use
>   *
>   * This function copies the skb with pskb_copy() and is sent as unicast packet
> @@ -563,7 +701,7 @@ batadv_dat_select_candidates(struct batadv_priv
> *bat_priv, __be32 ip_dst)
>   * otherwise.
>   */
>  static bool batadv_dat_send_data(struct batadv_priv *bat_priv,
> - struct sk_buff *skb, __be32 ip,
> + struct sk_buff *skb, void *ip, uint8_t ip_type,
>   int packet_subtype)
>  {
>   int i;
> @@ -573,11 +711,11 @@ static bool batadv_dat_send_data(struct
> batadv_priv *bat_priv,
>   struct sk_buff *tmp_skb;
>   struct batadv_dat_candidate *cand;
> 
> - cand = batadv_dat_select_candidates(bat_priv, ip);
> + cand = batadv_dat_select_candidates(bat_priv, ip, ip_type);
>   if (!cand)
>   goto out;
> 
> - batadv_dbg(BATADV_DBG_DAT, bat_priv, "DHT_SEND for %pI4\n", &ip);
> + batadv_dat_dbg_ip(bat_priv, ip_type, "DHT_SEND for ", "\n", ip);
> 
>   for (i = 0; i < BATADV_DAT_CANDIDATES_NUM; i++) {
>   if (cand[i].type == BATADV_DAT_CANDIDATE_NOT_FOUND)
> @@ -693,8 +831,8 @@ int batadv_dat_cache_seq_print_text(struct
> seq_file *seq, void *offset)
>   goto out;
> 
>   seq_printf(seq, "Distributed ARP Table (%s):\n", net_dev->name);
> - seq_printf(seq, "          %-7s          %-13s %5s\n", "IPv4", "MAC",
> -   "last-seen");
> + seq_printf(seq, "                       %-26s %-15s %5s\n",
> +   "IPv4/IPv6", "MAC", "last-seen");
> 
>   for (i = 0; i < hash->size; i++) {
>   head = &hash->table[i];
> @@ -707,9 +845,20 @@ int batadv_dat_cache_seq_print_text(struct
> seq_file *seq, void *offset)
>   last_seen_msecs = last_seen_msecs % 60000;
>   last_seen_secs = last_seen_msecs / 1000;
> 
> - seq_printf(seq, " * %15pI4 %14pM %6i:%02i\n",
> -   &dat_entry->ip, dat_entry->mac_addr,
> -   last_seen_mins, last_seen_secs);
> + switch (dat_entry->type) {
> + case BATADV_DAT_IPV4:
> + seq_printf(seq, " * %-40pI4 %15pM %6i:%02i\n",
> +   dat_entry->data, dat_entry->mac_addr,
> +   last_seen_mins, last_seen_secs);
> + break;
> +#if IS_ENABLED(CONFIG_IPV6)
> + case BATADV_DAT_IPV6:
> + seq_printf(seq, " * %-40pI6c %15pM %6i:%02i\n",
> +   dat_entry->data, dat_entry->mac_addr,
> +   last_seen_mins, last_seen_secs);
> + break;
> +#endif
> + }
>   }
>   rcu_read_unlock();
>   }
> @@ -830,9 +979,10 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
> batadv_priv *bat_priv,
>   hw_src = batadv_arp_hw_src(skb, 0);
>   ip_dst = batadv_arp_ip_dst(skb, 0);
> 
> - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> 
> - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> +       BATADV_DAT_IPV4);
>   if (dat_entry) {
>   skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src,
>       bat_priv->soft_iface, ip_dst, hw_src,
> @@ -851,8 +1001,9 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
> batadv_priv *bat_priv,
>   batadv_dbg(BATADV_DBG_DAT, bat_priv, "ARP request replied locally\n");
>   ret = true;
>   } else {
> - /* Send the request to the DHT */
> - ret = batadv_dat_send_data(bat_priv, skb, ip_dst,
> + /* Send the request on the DHT */
> + ret = batadv_dat_send_data(bat_priv, skb, &ip_dst,
> +   BATADV_DAT_IPV4,
>     BATADV_P_DAT_DHT_GET);
>   }
>  out:
> @@ -895,9 +1046,10 @@ bool
> batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv,
>   batadv_dbg_arp(bat_priv, skb, type, hdr_size,
>         "Parsing incoming ARP REQUEST");
> 
> - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> 
> - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> +       BATADV_DAT_IPV4);
>   if (!dat_entry)
>   goto out;
> 
> @@ -956,14 +1108,16 @@ void batadv_dat_snoop_outgoing_arp_reply(struct
> batadv_priv *bat_priv,
>   hw_dst = batadv_arp_hw_dst(skb, 0);
>   ip_dst = batadv_arp_ip_dst(skb, 0);
> 
> - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst);
> + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst);
> 
>   /* Send the ARP reply to the candidates for both the IP addresses that
>   * the node obtained from the ARP reply
>   */
> - batadv_dat_send_data(bat_priv, skb, ip_src, BATADV_P_DAT_DHT_PUT);
> - batadv_dat_send_data(bat_priv, skb, ip_dst, BATADV_P_DAT_DHT_PUT);
> + batadv_dat_send_data(bat_priv, skb, &ip_src, BATADV_DAT_IPV4,
> +     BATADV_P_DAT_DHT_PUT);
> + batadv_dat_send_data(bat_priv, skb, &ip_dst, BATADV_DAT_IPV4,
> +     BATADV_P_DAT_DHT_PUT);
>  }
>  /**
>   * batadv_dat_snoop_incoming_arp_reply - snoop the ARP reply and fill the local
> @@ -998,8 +1152,8 @@ bool batadv_dat_snoop_incoming_arp_reply(struct
> batadv_priv *bat_priv,
>   /* Update our internal cache with both the IP addresses the node got
>   * within the ARP reply
>   */
> - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst);
> + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst);
> 
>   /* if this REPLY is directed to a client of mine, let's deliver the
>   * packet to the interface
> @@ -1043,7 +1197,8 @@ bool batadv_dat_drop_broadcast_packet(struct
> batadv_priv *bat_priv,
>   goto out;
> 
>   ip_dst = batadv_arp_ip_dst(forw_packet->skb, bcast_len);
> - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> +       BATADV_DAT_IPV4);
>   /* check if the node already got this entry */
>   if (!dat_entry) {
>   batadv_dbg(BATADV_DBG_DAT, bat_priv,
> diff --git a/types.h b/types.h
> index 5f542bd..5b820f6 100644
> --- a/types.h
> +++ b/types.h
> @@ -961,17 +961,19 @@ struct batadv_algo_ops {
>  };
> 
>  /**
> - * struct batadv_dat_entry - it is a single entry of batman-adv ARP backend. It
> - * is used to stored ARP entries needed for the global DAT cache
> - * @ip: the IPv4 corresponding to this DAT/ARP entry
> - * @mac_addr: the MAC address associated to the stored IPv4
> + * struct batadv_dat_entry - it is a single entry of batman-adv
> ARP/NDP backend. It
> + * is used to stored ARP/NDP entries needed for the global DAT cache
> + * @data: the data corresponding to this DAT ARP/NDP entry
> + * @type: the type corresponding to this DAT ARP/NDP entry
> + * @mac_addr: the MAC address associated to the stored data
>   * @last_update: time in jiffies when this entry was refreshed last time
>   * @hash_entry: hlist node for batadv_priv_dat::hash
>   * @refcount: number of contexts the object is used
>   * @rcu: struct used for freeing in an RCU-safe manner
>   */
>  struct batadv_dat_entry {
> - __be32 ip;
> + void *data;
> + uint8_t type;
>   uint8_t mac_addr[ETH_ALEN];
>   unsigned long last_update;
>   struct hlist_node hash_entry;
> @@ -980,6 +982,18 @@ struct batadv_dat_entry {
>  };
> 
>  /**
> + * 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,
> +#if IS_ENABLED(CONFIG_IPV6)
> + BATADV_DAT_IPV6,
> +#endif
> +};
> +
> +/**
>   * 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
> --
> 1.7.10.4

Nice job so far!

My suggestion now is to send patch (1) for the new "data_to_str" function and then
split this RFC in two patches:
(2) generalise the dat_entry struct by introducing the type and data fields,
with all the needed functions
(3) introduce the IPv6 data type and the related function.

However, having patch (3) merged without a "user" would not make much sense.
So I think it would be good for now to send patch (1) and (2) and then send (3)
along with the "snooping patch" as soon as it is ready.

Thanks a lot!

Cheers,
  

On 19 April 2013 15:38, Antonio Quartulli <ordex@autistici.org> wrote:
>
> Hi Mihail,
>
> On Mon, Apr 15, 2013 at 12:37:33PM +0300, Mihail Costea wrote:
>
> [...]
>
> >
> > +#ifdef CONFIG_BATMAN_ADV_DEBUG
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +
> > +/* batadv_dat_dbg_ip - print similar debug message for IPv4 and IPv6.
> > + * @bat_priv: the bat priv with all the soft interface information
> > + * @ip_type: IPv4 / IPv6 address
> > + * @format_prefix: format before IP field
> > + * @format_suffix: format after IP field
> > + *
> > + * At list one variable parameter should be the IP itself, and it should
> > + * be placed correctly based on format prefix and format suffix arguments.
> > + */
> > +#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; \
> > + } \
> > + }
>
> I think this define is pretty overkill..What about creating a function which
> only takes care of converting a generic DHT data in a string. Later you can
> invoke this function when you want to print an IPv4/6 using batadv_dbg.
>
> Here is an example:
>
> char *batadv_dat_data_to_str(struct batadv_dat_entry *dat_entry,
>                              const char *buf, size_t buf_len)
> {
>         /* do something and put the string representation of the entry in the
>          * buf, without exceeding buf_len.
>          * Remember to use IS_ENABLED(CONFIG_IPV6) inside
>          */
>
>
>         return buf;
> }
>
> Then you can call it directly as parameter of a batdv_dbg function. E.g:
>
> batadv_dbg(....," %s\n", batadv_dat_data_to_str(entry, buf, buf_len));
>
> I think this approach would be better and easy to extend for future data types.
> If you like this approach, what do you think about sending a patch now to add
> this function to the current code?
>

This could work. buf could be a string with a given pattern inside
(let's say %data, or something similar) that would be automatically
converted to the right type (like %pI4, or %pI6c.

> > +
> >  static void batadv_dat_purge(struct work_struct *work);
> >
> >  /**
> > @@ -45,6 +101,19 @@ static void batadv_dat_start_timer(struct
> > batadv_priv *bat_priv)
> >  }
> >
> >  /**
> > + * batadv_dat_entry_free_ref_rcu - free a dat entry using its rcu
> > + * @rcu: the dat entry rcu
> > + */
> > +static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu)
> > +{
> > + struct batadv_dat_entry *dat_entry;
> > +
> > + dat_entry = container_of(rcu, struct batadv_dat_entry, rcu);
> > + kfree(dat_entry->data);
> > + kfree(dat_entry);
> > +}
> > +
> > +/**
> >   * batadv_dat_entry_free_ref - decrement the dat_entry refcounter and possibly
> >   * free it
> >   * @dat_entry: the entry to free
> > @@ -52,7 +121,7 @@ static void batadv_dat_start_timer(struct
> > batadv_priv *bat_priv)
> >  static void batadv_dat_entry_free_ref(struct batadv_dat_entry *dat_entry)
> >  {
> >   if (atomic_dec_and_test(&dat_entry->refcount))
> > - kfree_rcu(dat_entry, rcu);
> > + call_rcu(&dat_entry->rcu, batadv_dat_entry_free_ref_rcu);
> >  }
> >
> >  /**
> > @@ -130,6 +199,26 @@ static void batadv_dat_purge(struct work_struct *work)
> >  }
> >
> >  /**
> > + * batadv_sizeof_ip - get 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.
> > + */
> > +static size_t batadv_sizeof_ip(uint8_t ip_type)
> > +{
> > + switch (ip_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 sizeof(__be32); /* fallback to IPv4 */
> > + }
> > +}
>
> Either you mail client killed the tabs or this switch is not indented properly.
> Also, do you think it would be better to return 0 in case of unknown type? In
> this way we prevent any further operation on the data buffer.
>

It might be from my gmail. Next time I will send it with git send-email. Thanks.

> > +
> > +/**
> >   * batadv_compare_dat - comparing function used in the local DAT hash table
> >   * @node: node in the local table
> >   * @data2: second object to compare the node to
> > @@ -138,10 +227,18 @@ static void batadv_dat_purge(struct work_struct *work)
> >   */
> >  static int batadv_compare_dat(const struct hlist_node *node, const void *data2)
> >  {
> > - const void *data1 = container_of(node, struct batadv_dat_entry,
> > - hash_entry);
> > + struct batadv_dat_entry *dat_entry1 = container_of(node,
> > + struct batadv_dat_entry, hash_entry);
> > + struct batadv_dat_entry *dat_entry2 = container_of(data2,
> > + struct batadv_dat_entry, data);
> > + size_t ip_size;
> >
> > - return (memcmp(data1, data2, sizeof(__be32)) == 0 ? 1 : 0);
> > + if (dat_entry1->type != dat_entry2->type)
> > + return 0;
> > +
> > + ip_size = batadv_sizeof_ip(dat_entry1->type);
> > + return (memcmp(dat_entry1->data, dat_entry2->data,
> > +       ip_size) == 0 ? 1 : 0);
> >  }
> >
> >  /**
> > @@ -201,16 +298,19 @@ static __be32 batadv_arp_ip_dst(struct sk_buff
> > *skb, int hdr_size)
> >   * batadv_hash_dat - compute the hash value for an IP address
> >   * @data: data to hash
> >   * @size: size of the hash table
> > + * @ip_type: type of IP address
> >   *
> >   * Returns the selected index in the hash table for the given data.
> >   */
> > -static uint32_t batadv_hash_dat(const void *data, uint32_t size)
> > +static uint32_t batadv_hash_dat(const void *data, uint32_t size,
> > +       uint8_t ip_type)
> >  {
> >   const unsigned char *key = data;
> >   uint32_t hash = 0;
> > - size_t i;
> > + size_t i, ip_size;
> >
> > - for (i = 0; i < 4; i++) {
> > + ip_size = batadv_sizeof_ip(ip_type);
> > + for (i = 0; i < ip_size; i++) {
> >   hash += key[i];
> >   hash += (hash << 10);
> >   hash ^= (hash >> 6);
> > @@ -223,31 +323,47 @@ static uint32_t batadv_hash_dat(const void
> > *data, uint32_t size)
> >   return hash % size;
> >  }
>
> Also here, I think it is a mail client problem at this point :)
> Did you send the patch using git send-email? It will take care of not ruining
> the text ;)
>
> >
> > +static uint32_t batadv_hash_dat_ipv4(const void *data, uint32_t size)
> > +{
> > + return batadv_hash_dat(data, size, BATADV_DAT_IPV4);
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_IPV6)
> > +static uint32_t batadv_hash_dat_ipv6(const void *data, uint32_t size)
> > +{
> > + return batadv_hash_dat(data, size, BATADV_DAT_IPV6);
> > +}
> > +#endif
> > +
> >  /**
> >   * batadv_dat_entry_hash_find - look for a given dat_entry in the local hash
> >   * table
> >   * @bat_priv: the bat priv with all the soft interface information
> >   * @ip: search key
> > + * @ip_type: type of IP address
> >   *
> >   * Returns the dat_entry if found, NULL otherwise.
> >   */
> >  static struct batadv_dat_entry *
> > -batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, __be32 ip)
> > +batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *ip,
> > +   uint8_t ip_type)
> >  {
> >   struct hlist_head *head;
> >   struct batadv_dat_entry *dat_entry, *dat_entry_tmp = NULL;
> >   struct batadv_hashtable *hash = bat_priv->dat.hash;
> >   uint32_t index;
> > + size_t ip_size;
> >
> >   if (!hash)
> >   return NULL;
> >
> > - index = batadv_hash_dat(&ip, hash->size);
> > + ip_size = batadv_sizeof_ip(ip_type);
> > + index = batadv_hash_dat(ip, hash->size, ip_type);
> >   head = &hash->table[index];
> >
> >   rcu_read_lock();
> >   hlist_for_each_entry_rcu(dat_entry, head, hash_entry) {
> > - if (dat_entry->ip != ip)
> > + if (memcmp(dat_entry->data, ip, ip_size))
> >   continue;
> >
>
> Why don't you check the type first? if dat_entry->type != ip_type then you can
> "continue;" and skip the memory check. If you don't do so you may do a memory
> check beyond the limit of dat_entry->data if it is smaller than ip_size.
>

Thanks. Didn't think about this case.

> >   if (!atomic_inc_not_zero(&dat_entry->refcount))
> > @@ -264,24 +380,28 @@ batadv_dat_entry_hash_find(struct batadv_priv
> > *bat_priv, __be32 ip)
> >  /**
> >   * batadv_dat_entry_add - add a new dat entry or update it if already exists
> >   * @bat_priv: the bat priv with all the soft interface information
> > - * @ip: ipv4 to add/edit
> > + * @ip: IP to add/edit
> > + * @ip_type: type of IP address
> >   * @mac_addr: mac address to assign to the given ipv4
> >   */
> > -static void batadv_dat_entry_add(struct batadv_priv *bat_priv, __be32 ip,
> > - uint8_t *mac_addr)
> > +static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *ip,
> > + uint8_t ip_type, uint8_t *mac_addr)
> >  {
> >   struct batadv_dat_entry *dat_entry;
> >   int hash_added;
> > + size_t ip_size;
> > + batadv_hashdata_choose_cb choose;
> >
> > - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip);
> > + dat_entry = batadv_dat_entry_hash_find(bat_priv, ip, ip_type);
> >   /* if this entry is already known, just update it */
> >   if (dat_entry) {
> >   if (!batadv_compare_eth(dat_entry->mac_addr, mac_addr))
> >   memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN);
> >   dat_entry->last_update = jiffies;
> > - batadv_dbg(BATADV_DBG_DAT, bat_priv,
> > -   "Entry updated: %pI4 %pM\n", &dat_entry->ip,
> > -   dat_entry->mac_addr);
> > +
> > + batadv_dat_dbg_ip(bat_priv, ip_type, "Entry updated: ",
> > +  " %pM\n", dat_entry->data,
> > +  dat_entry->mac_addr);
> >   goto out;
> >   }
> >
> > @@ -289,13 +409,30 @@ static void batadv_dat_entry_add(struct
> > batadv_priv *bat_priv, __be32 ip,
> >   if (!dat_entry)
> >   goto out;
> >
> > - dat_entry->ip = ip;
> > + ip_size = batadv_sizeof_ip(ip_type);
> > + dat_entry->data = kmalloc(ip_size, GFP_ATOMIC);
> > + if (!dat_entry->data)
> > + goto out;
> > + memcpy(dat_entry->data, ip, ip_size);
> > + dat_entry->type = ip_type;
> > +
> >   memcpy(dat_entry->mac_addr, mac_addr, ETH_ALEN);
> >   dat_entry->last_update = jiffies;
> >   atomic_set(&dat_entry->refcount, 2);
> >
> > + switch (ip_type) {
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + case BATADV_DAT_IPV6:
> > + choose = batadv_hash_dat_ipv6;
> > + break;
> > +#endif
> > + default:
> > + choose = batadv_hash_dat_ipv4; /* IPv4 by default */
> > + break;
> > + }
> > +
> >   hash_added = batadv_hash_add(bat_priv->dat.hash, batadv_compare_dat,
> > -     batadv_hash_dat, &dat_entry->ip,
> > +     choose, dat_entry->data,
> >       &dat_entry->hash_entry);
> >
> >   if (unlikely(hash_added != 0)) {
> > @@ -304,9 +441,8 @@ static void batadv_dat_entry_add(struct
> > batadv_priv *bat_priv, __be32 ip,
> >   goto out;
> >   }
> >
> > - batadv_dbg(BATADV_DBG_DAT, bat_priv, "New entry added: %pI4 %pM\n",
> > -   &dat_entry->ip, dat_entry->mac_addr);
> > -
> > + batadv_dat_dbg_ip(bat_priv, ip_type, "New entry added: ", " %pM\n",
> > +  dat_entry->data, dat_entry->mac_addr);
>
> Here you can use the "new function" I told you about at the beginning of the
> email.
>
> >  out:
> >   if (dat_entry)
> >   batadv_dat_entry_free_ref(dat_entry);
> > @@ -513,7 +649,8 @@ static void batadv_choose_next_candidate(struct
> > batadv_priv *bat_priv,
> >   * batadv_dat_select_candidates - select the nodes which the DHT message has to
> >   * be sent to
> >   * @bat_priv: the bat priv with all the soft interface information
> > - * @ip_dst: ipv4 to look up in the DHT
> > + * @ip_dst: IP to look up in the DHT
> > + * @ip_type: type of IP address
> >   *
> >   * An originator O is selected if and only if its DHT_ID value is one of three
> >   * closest values (from the LEFT, with wrap around if needed) then the hash
> > @@ -522,7 +659,8 @@ static void batadv_choose_next_candidate(struct
> > batadv_priv *bat_priv,
> >   * Returns the candidate array of size BATADV_DAT_CANDIDATE_NUM.
> >   */
> >  static struct batadv_dat_candidate *
> > -batadv_dat_select_candidates(struct batadv_priv *bat_priv, __be32 ip_dst)
> > +batadv_dat_select_candidates(struct batadv_priv *bat_priv, void *ip_dst,
> > +     uint8_t ip_type)
> >  {
> >   int select;
> >   batadv_dat_addr_t last_max = BATADV_DAT_ADDR_MAX, ip_key;
> > @@ -535,12 +673,11 @@ batadv_dat_select_candidates(struct batadv_priv
> > *bat_priv, __be32 ip_dst)
> >   if (!res)
> >   return NULL;
> >
> > - ip_key = (batadv_dat_addr_t)batadv_hash_dat(&ip_dst,
> > -    BATADV_DAT_ADDR_MAX);
> > + ip_key = (batadv_dat_addr_t)batadv_hash_dat(ip_dst, BATADV_DAT_ADDR_MAX,
> > + ip_type);
> >
> > - batadv_dbg(BATADV_DBG_DAT, bat_priv,
> > -   "dat_select_candidates(): IP=%pI4 hash(IP)=%u\n", &ip_dst,
> > -   ip_key);
> > + batadv_dat_dbg_ip(bat_priv, ip_type, "dat_select_candidates(): IP=",
> > +  " hash(IP)=%u\n", ip_dst, ip_key);
> >
> >   for (select = 0; select < BATADV_DAT_CANDIDATES_NUM; select++)
> >   batadv_choose_next_candidate(bat_priv, res, select, ip_key,
> > @@ -554,6 +691,7 @@ batadv_dat_select_candidates(struct batadv_priv
> > *bat_priv, __be32 ip_dst)
> >   * @bat_priv: the bat priv with all the soft interface information
> >   * @skb: payload to send
> >   * @ip: the DHT key
> > + * @ip_type: type of IP address
> >   * @packet_subtype: unicast4addr packet subtype to use
> >   *
> >   * This function copies the skb with pskb_copy() and is sent as unicast packet
> > @@ -563,7 +701,7 @@ batadv_dat_select_candidates(struct batadv_priv
> > *bat_priv, __be32 ip_dst)
> >   * otherwise.
> >   */
> >  static bool batadv_dat_send_data(struct batadv_priv *bat_priv,
> > - struct sk_buff *skb, __be32 ip,
> > + struct sk_buff *skb, void *ip, uint8_t ip_type,
> >   int packet_subtype)
> >  {
> >   int i;
> > @@ -573,11 +711,11 @@ static bool batadv_dat_send_data(struct
> > batadv_priv *bat_priv,
> >   struct sk_buff *tmp_skb;
> >   struct batadv_dat_candidate *cand;
> >
> > - cand = batadv_dat_select_candidates(bat_priv, ip);
> > + cand = batadv_dat_select_candidates(bat_priv, ip, ip_type);
> >   if (!cand)
> >   goto out;
> >
> > - batadv_dbg(BATADV_DBG_DAT, bat_priv, "DHT_SEND for %pI4\n", &ip);
> > + batadv_dat_dbg_ip(bat_priv, ip_type, "DHT_SEND for ", "\n", ip);
> >
> >   for (i = 0; i < BATADV_DAT_CANDIDATES_NUM; i++) {
> >   if (cand[i].type == BATADV_DAT_CANDIDATE_NOT_FOUND)
> > @@ -693,8 +831,8 @@ int batadv_dat_cache_seq_print_text(struct
> > seq_file *seq, void *offset)
> >   goto out;
> >
> >   seq_printf(seq, "Distributed ARP Table (%s):\n", net_dev->name);
> > - seq_printf(seq, "          %-7s          %-13s %5s\n", "IPv4", "MAC",
> > -   "last-seen");
> > + seq_printf(seq, "                       %-26s %-15s %5s\n",
> > +   "IPv4/IPv6", "MAC", "last-seen");
> >
> >   for (i = 0; i < hash->size; i++) {
> >   head = &hash->table[i];
> > @@ -707,9 +845,20 @@ int batadv_dat_cache_seq_print_text(struct
> > seq_file *seq, void *offset)
> >   last_seen_msecs = last_seen_msecs % 60000;
> >   last_seen_secs = last_seen_msecs / 1000;
> >
> > - seq_printf(seq, " * %15pI4 %14pM %6i:%02i\n",
> > -   &dat_entry->ip, dat_entry->mac_addr,
> > -   last_seen_mins, last_seen_secs);
> > + switch (dat_entry->type) {
> > + case BATADV_DAT_IPV4:
> > + seq_printf(seq, " * %-40pI4 %15pM %6i:%02i\n",
> > +   dat_entry->data, dat_entry->mac_addr,
> > +   last_seen_mins, last_seen_secs);
> > + break;
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + case BATADV_DAT_IPV6:
> > + seq_printf(seq, " * %-40pI6c %15pM %6i:%02i\n",
> > +   dat_entry->data, dat_entry->mac_addr,
> > +   last_seen_mins, last_seen_secs);
> > + break;
> > +#endif
> > + }
> >   }
> >   rcu_read_unlock();
> >   }
> > @@ -830,9 +979,10 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
> > batadv_priv *bat_priv,
> >   hw_src = batadv_arp_hw_src(skb, 0);
> >   ip_dst = batadv_arp_ip_dst(skb, 0);
> >
> > - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> > + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> >
> > - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> > + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> > +       BATADV_DAT_IPV4);
> >   if (dat_entry) {
> >   skb_new = arp_create(ARPOP_REPLY, ETH_P_ARP, ip_src,
> >       bat_priv->soft_iface, ip_dst, hw_src,
> > @@ -851,8 +1001,9 @@ bool batadv_dat_snoop_outgoing_arp_request(struct
> > batadv_priv *bat_priv,
> >   batadv_dbg(BATADV_DBG_DAT, bat_priv, "ARP request replied locally\n");
> >   ret = true;
> >   } else {
> > - /* Send the request to the DHT */
> > - ret = batadv_dat_send_data(bat_priv, skb, ip_dst,
> > + /* Send the request on the DHT */
> > + ret = batadv_dat_send_data(bat_priv, skb, &ip_dst,
> > +   BATADV_DAT_IPV4,
> >     BATADV_P_DAT_DHT_GET);
> >   }
> >  out:
> > @@ -895,9 +1046,10 @@ bool
> > batadv_dat_snoop_incoming_arp_request(struct batadv_priv *bat_priv,
> >   batadv_dbg_arp(bat_priv, skb, type, hdr_size,
> >         "Parsing incoming ARP REQUEST");
> >
> > - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> > + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> >
> > - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> > + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> > +       BATADV_DAT_IPV4);
> >   if (!dat_entry)
> >   goto out;
> >
> > @@ -956,14 +1108,16 @@ void batadv_dat_snoop_outgoing_arp_reply(struct
> > batadv_priv *bat_priv,
> >   hw_dst = batadv_arp_hw_dst(skb, 0);
> >   ip_dst = batadv_arp_ip_dst(skb, 0);
> >
> > - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> > - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst);
> > + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> > + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst);
> >
> >   /* Send the ARP reply to the candidates for both the IP addresses that
> >   * the node obtained from the ARP reply
> >   */
> > - batadv_dat_send_data(bat_priv, skb, ip_src, BATADV_P_DAT_DHT_PUT);
> > - batadv_dat_send_data(bat_priv, skb, ip_dst, BATADV_P_DAT_DHT_PUT);
> > + batadv_dat_send_data(bat_priv, skb, &ip_src, BATADV_DAT_IPV4,
> > +     BATADV_P_DAT_DHT_PUT);
> > + batadv_dat_send_data(bat_priv, skb, &ip_dst, BATADV_DAT_IPV4,
> > +     BATADV_P_DAT_DHT_PUT);
> >  }
> >  /**
> >   * batadv_dat_snoop_incoming_arp_reply - snoop the ARP reply and fill the local
> > @@ -998,8 +1152,8 @@ bool batadv_dat_snoop_incoming_arp_reply(struct
> > batadv_priv *bat_priv,
> >   /* Update our internal cache with both the IP addresses the node got
> >   * within the ARP reply
> >   */
> > - batadv_dat_entry_add(bat_priv, ip_src, hw_src);
> > - batadv_dat_entry_add(bat_priv, ip_dst, hw_dst);
> > + batadv_dat_entry_add(bat_priv, &ip_src, BATADV_DAT_IPV4, hw_src);
> > + batadv_dat_entry_add(bat_priv, &ip_dst, BATADV_DAT_IPV4, hw_dst);
> >
> >   /* if this REPLY is directed to a client of mine, let's deliver the
> >   * packet to the interface
> > @@ -1043,7 +1197,8 @@ bool batadv_dat_drop_broadcast_packet(struct
> > batadv_priv *bat_priv,
> >   goto out;
> >
> >   ip_dst = batadv_arp_ip_dst(forw_packet->skb, bcast_len);
> > - dat_entry = batadv_dat_entry_hash_find(bat_priv, ip_dst);
> > + dat_entry = batadv_dat_entry_hash_find(bat_priv, &ip_dst,
> > +       BATADV_DAT_IPV4);
> >   /* check if the node already got this entry */
> >   if (!dat_entry) {
> >   batadv_dbg(BATADV_DBG_DAT, bat_priv,
> > diff --git a/types.h b/types.h
> > index 5f542bd..5b820f6 100644
> > --- a/types.h
> > +++ b/types.h
> > @@ -961,17 +961,19 @@ struct batadv_algo_ops {
> >  };
> >
> >  /**
> > - * struct batadv_dat_entry - it is a single entry of batman-adv ARP backend. It
> > - * is used to stored ARP entries needed for the global DAT cache
> > - * @ip: the IPv4 corresponding to this DAT/ARP entry
> > - * @mac_addr: the MAC address associated to the stored IPv4
> > + * struct batadv_dat_entry - it is a single entry of batman-adv
> > ARP/NDP backend. It
> > + * is used to stored ARP/NDP entries needed for the global DAT cache
> > + * @data: the data corresponding to this DAT ARP/NDP entry
> > + * @type: the type corresponding to this DAT ARP/NDP entry
> > + * @mac_addr: the MAC address associated to the stored data
> >   * @last_update: time in jiffies when this entry was refreshed last time
> >   * @hash_entry: hlist node for batadv_priv_dat::hash
> >   * @refcount: number of contexts the object is used
> >   * @rcu: struct used for freeing in an RCU-safe manner
> >   */
> >  struct batadv_dat_entry {
> > - __be32 ip;
> > + void *data;
> > + uint8_t type;
> >   uint8_t mac_addr[ETH_ALEN];
> >   unsigned long last_update;
> >   struct hlist_node hash_entry;
> > @@ -980,6 +982,18 @@ struct batadv_dat_entry {
> >  };
> >
> >  /**
> > + * 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,
> > +#if IS_ENABLED(CONFIG_IPV6)
> > + BATADV_DAT_IPV6,
> > +#endif
> > +};
> > +
> > +/**
> >   * 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
> > --
> > 1.7.10.4
>
> Nice job so far!
>
> My suggestion now is to send patch (1) for the new "data_to_str" function and then
> split this RFC in two patches:
> (2) generalise the dat_entry struct by introducing the type and data fields,
> with all the needed functions
> (3) introduce the IPv6 data type and the related function.
>
> However, having patch (3) merged without a "user" would not make much sense.
> So I think it would be good for now to send patch (1) and (2) and then send (3)
> along with the "snooping patch" as soon as it is ready.
>
> Thanks a lot!
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Thanks for the advice. I will make the changes asap.

Mihail
  

On Fri, Apr 19, 2013 at 09:39:17 +0300, Mihail Costea wrote:
> On 19 April 2013 15:38, Antonio Quartulli <ordex@autistici.org> wrote:
> >
> > I think this define is pretty overkill..What about creating a function which
> > only takes care of converting a generic DHT data in a string. Later you can
> > invoke this function when you want to print an IPv4/6 using batadv_dbg.
> >
> > Here is an example:
> >
> > char *batadv_dat_data_to_str(struct batadv_dat_entry *dat_entry,
> >                              const char *buf, size_t buf_len)
> > {
> >         /* do something and put the string representation of the entry in the
> >          * buf, without exceeding buf_len.
> >          * Remember to use IS_ENABLED(CONFIG_IPV6) inside
> >          */
> >
> >
> >         return buf;
> > }
> >
> > Then you can call it directly as parameter of a batdv_dbg function. E.g:
> >
> > batadv_dbg(....," %s\n", batadv_dat_data_to_str(entry, buf, buf_len));
> >
> > I think this approach would be better and easy to extend for future data types.
> > If you like this approach, what do you think about sending a patch now to add
> > this function to the current code?
> >
> 
> This could work. buf could be a string with a given pattern inside
> (let's say %data, or something similar) that would be automatically
> converted to the right type (like %pI4, or %pI6c.

mh..I did not quite get this..If you know how to implement this "pattern
substitution" you can send a patch with that. Otherwise what I proposed above is
just another way of doing that, which is at least simpler and easier to read
than the macro you proposed before (imho)


Cheers,
  

On 20 April 2013 16:56, Antonio Quartulli <ordex@autistici.org> wrote:
> On Fri, Apr 19, 2013 at 09:39:17 +0300, Mihail Costea wrote:
>> On 19 April 2013 15:38, Antonio Quartulli <ordex@autistici.org> wrote:
>> >
>> > I think this define is pretty overkill..What about creating a function which
>> > only takes care of converting a generic DHT data in a string. Later you can
>> > invoke this function when you want to print an IPv4/6 using batadv_dbg.
>> >
>> > Here is an example:
>> >
>> > char *batadv_dat_data_to_str(struct batadv_dat_entry *dat_entry,
>> >                              const char *buf, size_t buf_len)
>> > {
>> >         /* do something and put the string representation of the entry in the
>> >          * buf, without exceeding buf_len.
>> >          * Remember to use IS_ENABLED(CONFIG_IPV6) inside
>> >          */
>> >
>> >
>> >         return buf;
>> > }
>> >
>> > Then you can call it directly as parameter of a batdv_dbg function. E.g:
>> >
>> > batadv_dbg(....," %s\n", batadv_dat_data_to_str(entry, buf, buf_len));
>> >
>> > I think this approach would be better and easy to extend for future data types.
>> > If you like this approach, what do you think about sending a patch now to add
>> > this function to the current code?
>> >
>>
>> This could work. buf could be a string with a given pattern inside
>> (let's say %data, or something similar) that would be automatically
>> converted to the right type (like %pI4, or %pI6c.
>
> mh..I did not quite get this..If you know how to implement this "pattern
> substitution" you can send a patch with that. Otherwise what I proposed above is
> just another way of doing that, which is at least simpler and easier to read
> than the macro you proposed before (imho)
>
>
> Cheers,
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

I think I just understood what you meant. Use that function just for
the data member of dat_entry and in rest keep everything as it is.
That makes stuff easier than the way I thought it.

Thanks,
Mihail Costea