Hi Antonio,
On Sun, May 12, 2013 at 12:55:25AM +0200, Antonio Quartulli wrote:
Hey Linus,
here you have some comments inline
On Sat, May 11, 2013 at 07:23:25PM +0200, Linus Lüssing wrote:
diff --git a/main.h b/main.h index 795345f..39c683d 100644 --- a/main.h +++ b/main.h @@ -141,6 +141,14 @@ enum batadv_uev_type { /* Append 'batman-adv: ' before kernel messages */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define UINT8_MAX 255
I think you should define it as 255U, otherwise it would get the int type and may generate warnings somewhere (or also unexpected result sometime..)
ok
+/* identical to the one used in net/ipv4/igmp.c */ +#define for_each_pmc_rcu(in_dev, pmc) \
- for (pmc = rcu_dereference(in_dev->mc_list); \
pmc != NULL; \pmc = rcu_dereference(pmc->next_rcu))Since it is exactly the same code reported in net/ipv4/igmp.c and since it is a define, don't you thin kit is worth trying to send a patch to netdev to move the define in a proper header file so that we/everybody can re-use it?
Yes. Hm, I think I'd prefer adding the potential user in netdev first and then moving it to a header file. I'll add an explicit 'TODO' in the comment.
+/**
- batadv_mcast_mla_local_collect - Collects local multicast listeners
please start the description with a small letter (like we do everywhere..just keep the same style)
- @dev: The device to collect multicast addresses from
don't put tabs between the name of the arg and the description. don't use the capital letter. Just:
@dev: the device..
would be nice :)
ok
+static int batadv_mcast_mla_local_collect(struct net_device *dev,
struct list_head *mcast_list,int num_mla_max) {The opening parenthesis of a function must go at the beginning of the next line.
- struct netdev_hw_addr *mc_list_entry, *new;
- int num_mla = 0, ret = 0;
- netif_addr_lock_bh(dev);
- netdev_for_each_mc_addr(mc_list_entry, dev) {
if (num_mla >= num_mla_max) {pr_warn("Too many local multicast listener announcements here, just adding %i\n",num_mla_max);why pr_warn and not just a debug message? Is this a symptom of a possible malfunctioning?
It was intended as a warning for a malfunction. But actually - I think I can remove it and the limitation of number of entries, it's more a relic of the old announcing approach. It should be up to the TT mechanism to issue such warnings now instead.
break;}new = kmalloc(sizeof(struct netdev_hw_addr), GFP_ATOMIC);please use sizeof(*new) here. we agreed on this some time ago. This helps because if we decide to change the type of new in the future then we do not need to change all the kmalloc invocations.
ok
+static bool batadv_mcast_mla_is_duplicate(uint8_t *mcast_addr,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry;
- list_for_each_entry(mcast_entry, mcast_list, list)
if (!memcmp(mcast_entry->addr, mcast_addr, ETH_ALEN))in main.h we have batadv_compare_eth(), you can re-use it here
ok
+static void batadv_mcast_mla_tt_clean(struct batadv_priv *bat_priv,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- list_for_each_entry_safe(mcast_entry, tmp, &bat_priv->mcast.mla_list,
list) {if (batadv_mcast_mla_is_duplicate(mcast_entry->addr,mcast_list))continue;batadv_tt_local_remove(bat_priv, mcast_entry->addr,"mcast TT outdated", 0);^^^parameter is bool, please use "false" instead of 0
ok
+static void batadv_mcast_mla_tt_add(struct net_device *soft_iface,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- struct batadv_priv *bat_priv = netdev_priv(soft_iface);
why not simply pass bat_priv as first argument (as you do in all the other functions) and then use bat_priv->soft_iface when needed ? (this soft_iface member has been added recently to save code for lazy people :))
ok
+/**
- batadv_mcast_mla_tt_update - Updates the own MLAs
- @bat_priv: the bat priv with all the soft interface information
- Updates the own multicast listener announcements in the translation
For some reason we agreed on not using the third person while describing the function. For consistency I'd suggest you to do the same your kernel doc.
I don't quite understand what you mean, talking in the first or second person seems strange, like "I update the multicast..." or "You update the multicast...". Do you have an example?
- table. Also takes care of registering or unregistering the multicast
- tvlv depending on whether the user activated or deactivated
- multicast optimizations.
- */
+void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) +{
- struct batadv_hard_iface *primary_if;
- struct net_device *soft_iface;
- struct list_head mcast_list;
- int ret;
- static bool enabled;
- INIT_LIST_HEAD(&mcast_list);
- primary_if = batadv_primary_if_get_selected(bat_priv);
- if (!primary_if)
goto out;- soft_iface = primary_if->soft_iface;
- /* Avoid attaching MLAs, if multicast optimization is disabled
* or there is a bridge on top of our soft interface (TODO) *//* comments should
- be closed
- like this
*/
/* not like
- this */
(this is something checkpatch should also tell you (but I guess only when your code is in the net/ folder inside the kernel tree...yes it is a netdev rule only :))
Ah! Didn't know about that, thanks. Yes, I was only checking raw git-format-patch'ed patch files from the batman repository.
+/**
- batadv_mcast_free - Frees the multicast optimizaitons structures
- @bat_priv: the bat priv with all the soft interface information
- */
+void batadv_mcast_free(struct batadv_priv *bat_priv) +{
- struct list_head mcast_list;
- INIT_LIST_HEAD(&mcast_list);
- batadv_mcast_mla_tt_clean(bat_priv, &mcast_list);
can't you make batadv_mcast_mla_tt_clean() take a NULL second argument and in that case skip the check in the loop? would be cleaner I guess? what do you think?
Sounds good, will change that!
diff --git a/translation-table.c b/translation-table.c index df6b5cd..37e7d47 100644 --- a/translation-table.c +++ b/translation-table.c @@ -26,6 +26,7 @@ #include "originator.h" #include "routing.h" #include "bridge_loop_avoidance.h" +#include "multicast.h"
#include <linux/crc32c.h>
@@ -281,7 +282,8 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, bool roamed_back = false;
tt_local = batadv_tt_local_hash_find(bat_priv, addr);
- tt_global = batadv_tt_global_hash_find(bat_priv, addr);
- tt_global = is_multicast_ether_addr(addr) ? NULL :
batadv_tt_global_hash_find(bat_priv, addr);what about:
tt_global = NULL; (you can put this directly in the declaration) if (!is_multicast_ether_addr(addr)) tt_global = batadv_tt_global_hash_find(bat_priv, addr);
it is a bit easier to read, no?
Yes.
if (tt_local) { tt_local->last_seen = jiffies; @@ -332,8 +334,10 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, tt_local->last_seen = jiffies; tt_local->common.added_at = tt_local->last_seen;
- /* the batman interface mac address should never be purged */
- if (batadv_compare_eth(addr, soft_iface->dev_addr))
- /* the batman interface mac and multicast addresses should never be
* purged */comment style like above
if (batadv_compare_eth(addr, soft_iface->dev_addr) ||
is_multicast_ether_addr(addr))tt_local->common.flags |= BATADV_TT_CLIENT_NOPURGE;
hash_added = batadv_hash_add(bat_priv->tt.local_hash, batadv_compare_tt,
@@ -919,6 +923,10 @@ add_orig_entry: ret = true;
out_remove:
/* Do not remove multicast addresses from the local hash on
* global additions */if (is_multicast_ether_addr(tt_addr))
goto out;/* remove address from local hash if present */ local_flags = batadv_tt_local_remove(bat_priv, tt_addr,
@@ -2346,6 +2354,9 @@ void batadv_tt_local_commit_changes(struct batadv_priv *bat_priv) { uint16_t changed_num = 0;
- /* Update multicast addresses in local translation table */
- batadv_mcast_mla_tt_update(bat_priv);
- if (atomic_read(&bat_priv->tt.local_changes) < 1) { if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt)) batadv_tt_tvlv_container_update(bat_priv);
diff --git a/types.h b/types.h index c84f5cc..5d73a75 100644 --- a/types.h +++ b/types.h @@ -473,6 +473,12 @@ struct batadv_priv_dat { }; #endif
+#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS +struct batadv_priv_mcast {
- struct list_head mla_list;
+}; +#endif
/**
- struct batadv_priv_nc - per mesh interface network coding private data
- @work: work queue callback item for cleanup
@@ -561,6 +567,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT atomic_t distributed_arp_table; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- atomic_t mcast_group_awareness;
+#endif atomic_t gw_mode; atomic_t gw_sel_class; atomic_t orig_interval; @@ -595,6 +604,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT struct batadv_priv_dat dat; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- struct batadv_priv_mcast mcast;
+#endif #ifdef CONFIG_BATMAN_ADV_NC atomic_t network_coding; struct batadv_priv_nc nc; -- 1.7.10.4
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Cheers, Linus