Re: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Modified forwarding behaviour for multicast packets

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

Only a few style suggestions here, nothing critical and you can ignore
them if you don't like them. :)

On Fri, May 24, 2013 at 10:02:28AM +0200, Linus Lüssing wrote:
> With this patch a multicast packet is not always simply flooded anymore,
> the bevahiour for the following cases is changed to reduce
> unnecessary overhead:
> 
> If all nodes within the horizon of a certain node have signalized
> multicast listener announcement capability
> (BATADV_MCAST_LISTENER_ANNOUNCEMENT) then an IPv6 multicast packet
> with a destination of IPv6 link-local scope coming from the upstream
> of this node...
> 
> * ...is dropped if there is no according multicast listener in the
>   translation table,
> * ...is forwarded via unicast if there is a single node with interested
>   multicast listeners
> * ...and otherwise still gets flooded.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  multicast.c         |   43 +++++++++++++++++++++++++++++++++
>  multicast.h         |    8 +++++++
>  soft-interface.c    |   10 ++++++++
>  translation-table.c |   66 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  translation-table.h |    1 +
>  5 files changed, 128 insertions(+)
> 
> diff --git a/multicast.c b/multicast.c
> index 36e4c59..bd55c8f 100644
> --- a/multicast.c
> +++ b/multicast.c
> @@ -213,6 +213,49 @@ out:
>  }
>  
>  /**
> + * batadv_mcast_flood - check on how to forward a multicast packet
> + * @skb: The multicast packet to check
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Return 1 if the packet should be flooded, 0 if it should be forwarded
> + * via unicast or -1 if it should be drooped.
> + */
> +int batadv_mcast_flood(struct sk_buff *skb, struct batadv_priv *bat_priv)
> +{
> +	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);
> +	struct ipv6hdr *ip6hdr;
> +	int count, ret = 1;
> +
> +	if (atomic_read(&bat_priv->mcast_group_awareness) &&
> +	    !atomic_read(&bat_priv->mcast.num_non_aware) &&
> +	    ntohs(ethhdr->h_proto) == ETH_P_IPV6) {

You can safe an indendation below if you return -1 immediately here
if the statement above is false. Also multiple statements might be better
for readability and later changes, e.g.

if (!atomic_read(&bat_priv->mcast_group_awareness))
	return 1;

if (atomic_read(&bat_priv->mcast.num_non_aware))
	return 1;

if (ntohs(ethhdr->h_proto) != ETH_P_IPV6)
	return 1;

> +		if (!pskb_may_pull(skb, sizeof(*ethhdr) + sizeof(*ip6hdr))) {
> +			ret = -1;
> +			goto out;
> +		}

You could directly return -1 here, the out label is not needed
(as we don't unlock/free anything here).

I don't quite understand why you return -1, maybe the packet could still
be forwarded even if it could not be pulled?

> +
> +		ip6hdr = ipv6_hdr(skb);
> +
> +		/* TODO: Implement Multicast Router Discovery, then add
> +		 * scope >= IPV6_ADDR_SCOPE_LINKLOCAL, too
> +		 */
> +		if (IPV6_ADDR_MC_SCOPE(&ip6hdr->daddr) !=
> +		    IPV6_ADDR_SCOPE_LINKLOCAL)
> +			goto out;
> +
> +		count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest);
> +
> +		if (!count)
> +			ret = -1;
> +		else if (count == 1)
> +			ret = 0;
> +	}

You could use a switch statement here instead for readability, e.g.:

switch (count) {
case 0: 
	return -1;
case 1:
	return 0;
default:
	return 1;
}
> +
> +out:
> +	return ret;
> +}
> +
> +/**
>   * batadv_mcast_tvlv_ogm_handler_v1 - process incoming multicast tvlv container
>   * @bat_priv: the bat priv with all the soft interface information
>   * @orig: the orig_node of the ogm
> diff --git a/soft-interface.c b/soft-interface.c
> index 8bdd649..83e4679 100644
> --- a/soft-interface.c
> +++ b/soft-interface.c
> @@ -36,6 +36,7 @@
>  #include <linux/if_vlan.h>
>  #include <linux/if_ether.h>
>  #include "unicast.h"
> +#include "multicast.h"
>  #include "bridge_loop_avoidance.h"
>  #include "network-coding.h"
>  
> @@ -222,6 +223,15 @@ static int batadv_interface_tx(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (do_bcast && !is_broadcast_ether_addr(ethhdr->h_dest)) {

I'd suggest to put this inside the "is_multicast_etheraddr()" above to make more clear
that this handles multicast packets. I was a little confused by the
do_bcast && !is_broadcast_ether_addr() first, but that might just be me.

Cheers,
	Simon

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