On Mon, Jul 08, 2013 at 03:12:45AM +0300, mihail.costea2005@gmail.com wrote: > From: Mihail Costea > > Snoops router and override flags for Neighbor Advertisement in order to > use it in NA creation. This information is stored in a extra member > in the dat entry. This is done separately from normal data, > because data is used for DHT comparison, while this flags can change > their value over time. Ok, therefore the extra member is not part of the "key" ( that we wrongly called data till now), but it is part of the "data" (that was a mac address only till now). I think that at this point it is better to generalise the data part too. We are not storing MAC addresses only anymore. For IPv4 we have data = { mac_addr } For IPv6 now we have data = { mac_addr, extra_stuff }. (and in the future we might have something else). I thought about not generalising the data field, but if we are going to introduce new (and IPv6 specific) fields than we have to do it anyway. I think that having a generic void *data and data_size where we can store the struct we want is much cleaner. What do you think? you think it is a too big work? In this case we could leave as you implemented it here and generalise later...Actually you "only" have to bring the mac_addr field inside the extra_data and rename extra_data to data. > > Comments: > For now router and override are initialized to the values used in most > case scenarios. This can be changed easily if we want to avoid the > NA creation until we get the flags. when do we get this flags? when we create the entry don't we get the flags too from the snooped packet? (sorry but I don't entirely know the ND protocol). > Also, the router flag can be calculated easily using the Router Advertisement. > Any node that sends that package is a router, so there isn't a lot of snooping > in that case. This feature can be added easily. > The problem is that I have no other idea how to get the override flag, > with the exception of the current implemented mechanism. > > Signed-off-by: Mihail Costea > Signed-off-by: Stefan Popa > Reviewed-by: Stefan Popa > > --- > distributed-arp-table.c | 139 +++++++++++++++++++++++++++++++++++------------ > types.h | 21 ++++++- > 2 files changed, 124 insertions(+), 36 deletions(-) > > diff --git a/distributed-arp-table.c b/distributed-arp-table.c > index 1a5749b..ce5c13d 100644 > --- a/distributed-arp-table.c > +++ b/distributed-arp-table.c > @@ -39,7 +39,8 @@ static bool batadv_dat_snoop_arp_pkt(struct batadv_priv *bat_priv, > struct sk_buff *skb, int hdr_size, > uint16_t pkt_type, uint8_t pkt_dir_type, > uint8_t **hw_src, void **ip_src, > - uint8_t **hw_dst, void **ip_dst); > + uint8_t **hw_dst, void **ip_dst, > + void **extra_data); > > static struct sk_buff * > batadv_dat_create_arp_reply(struct batadv_priv *bat_priv, > @@ -56,7 +57,8 @@ static bool batadv_dat_snoop_ndisc_pkt(struct batadv_priv *bat_priv, > struct sk_buff *skb, int hdr_size, > uint16_t pkt_type, uint8_t pkt_dir_type, > uint8_t **hw_src, void **ip_src, > - uint8_t **hw_dst, void **ip_dst); > + uint8_t **hw_dst, void **ip_dst, > + void **extra_data); > > static struct sk_buff * > batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv, > @@ -68,6 +70,7 @@ batadv_dat_create_ndisc_na(struct batadv_priv *bat_priv, > static struct batadv_dat_type_info batadv_dat_types_info[] = { > { > .size = sizeof(__be32), > + .extra_size = 0, > .str_fmt = "%pI4", > .snoop_pkt = batadv_dat_snoop_arp_pkt, > .create_skb = batadv_dat_create_arp_reply, > @@ -75,6 +78,7 @@ static struct batadv_dat_type_info batadv_dat_types_info[] = { > #if IS_ENABLED(CONFIG_IPV6) > { > .size = sizeof(struct in6_addr), > + .extra_size = sizeof(struct batadv_dat_ipv6_extra_data), > .str_fmt = "%pI6c", > .snoop_pkt = batadv_dat_snoop_ndisc_pkt, > .create_skb = batadv_dat_create_ndisc_na, > @@ -119,6 +123,7 @@ static void batadv_dat_entry_free_ref_rcu(struct rcu_head *rcu) > > dat_entry = container_of(rcu, struct batadv_dat_entry, rcu); > kfree(dat_entry->data); > + kfree(dat_entry->extra_data); > kfree(dat_entry); > } > > @@ -363,18 +368,20 @@ batadv_dat_entry_hash_find(struct batadv_priv *bat_priv, void *data, > * 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 > * @data: the data to add/edit > + * @extra_data: the extra data for data param (can be NULL if none) > * @data_type: type of the data added to DAT > * @mac_addr: mac address to assign to the given data > * @vid: VLAN identifier > */ > static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data, > - uint8_t data_type, uint8_t *mac_addr, > - unsigned short vid) > + void *extra_data, uint8_t data_type, > + uint8_t *mac_addr, unsigned short vid) > { > struct batadv_dat_entry *dat_entry; > int hash_added; > char dbg_data[BATADV_DAT_DATA_MAX_LEN]; > size_t data_size = batadv_dat_types_info[data_type].size; > + size_t extra_data_size = batadv_dat_types_info[data_type].extra_size; > > dat_entry = batadv_dat_entry_hash_find(bat_priv, data, data_type, vid); > /* if this entry is already known, just update it */ > @@ -382,22 +389,40 @@ static void batadv_dat_entry_add(struct batadv_priv *bat_priv, void *data, > 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: %s %pM (vid: %u)\n", > + if (extra_data) > + memcpy(dat_entry->extra_data, extra_data, > + extra_data_size); > + > + batadv_dbg(BATADV_DBG_DAT, bat_priv, > + "Entry updated: %s %pM (vid: %u)\n", > batadv_dat_data_to_str(dat_entry->data, data_type, > dbg_data, sizeof(dbg_data)), > dat_entry->mac_addr, vid); > goto out; > } > > + /* alloc entry */ > dat_entry = kmalloc(sizeof(*dat_entry), GFP_ATOMIC); > if (!dat_entry) > goto out; > + /* some entries don't have an extra data and useful if allocation for > + * data fails */ this comment has to be indented /* like * this */ There are other style issues in this patch, but they mostly concern what I already pointed out in the other comments. Remember to always check your patch with checkpatch.pl --strict in order to find problems before sending the patches. But overall is a very good job! I think we are close to the real patch series ;) Cheers, -- Antonio Quartulli ..each of us alone is worth nothing.. Ernesto "Che" Guevara