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

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Thursday 14 November 2013 07:26:07 Linus Lüssing wrote:
> +/**
> + * batadv_mcast_forw_mode_check - check for optimized forwarding potential
> + * @skb: the multicast frame to check
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Check whether the given multicast ethernet frame has the potential to
> + * be forwarded with a mode more optimal than classic flooding.
> + *
> + * If so then return 0. Otherwise -EINVAL is returned or -ENOMEM if we are
> + * out of memory.
> + */
> +static int batadv_mcast_forw_mode_check(struct sk_buff *skb,
> +					struct batadv_priv *bat_priv)
> +{
> +	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);

Please check if calling eth_hdr() is an option.
Another minor nitpick: When bat_priv and skb are arguments to the same 
function please keep bat_priv the first argument and skb the second for 
consistency purposes. You can check routing.c for other examples. 


> +/**
> + * batadv_mcast_forw_mode - 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 the forwarding mode as enum batadv_forw_mode.
> + */
> +enum batadv_forw_mode
> +batadv_mcast_forw_mode(struct sk_buff *skb, struct batadv_priv *bat_priv)
> +{
> +	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);

Same as above.


> +/**
> + * batadv_forw_mode - the way a packet should be forwarded as
> + * @BATADV_FORW_ALL: forward the packet to all nodes
> + *  (currently via classic flooding)
> + * @BATADV_FORW_SINGLE: forward the packet to a single node
> + *  (currently via the BATMAN_IV unicast routing protocol)

Can we scratch the 'BATMAN_IV routing protocol ? How about:
(currently via BATMAN unicast routing) ?


>  send:
> +		if (do_bcast && !is_broadcast_ether_addr(ethhdr->h_dest)) {
> +			forw_mode = batadv_mcast_forw_mode(skb, bat_priv);
> +			if (forw_mode == BATADV_FORW_NONE)
> +				goto dropped;
> +
> +			if (forw_mode == BATADV_FORW_SINGLE)
> +				do_bcast = false;
> +		}
> +	}

Has this been tested with gateway mode (IPv4 and IPv6) ? I can't come up with 
a case where it breaks but it also does not give me the feeling of being 100% 
safe. One option would be to add an additional dhcp_rcp variable check.


> diff --git a/types.h b/types.h
> index 39b9bbe..badbdf9 100644
> --- a/types.h
> +++ b/types.h
> @@ -712,6 +712,9 @@ struct batadv_priv {
>  #ifdef CONFIG_BATMAN_ADV_DAT
>  	atomic_t distributed_arp_table;
>  #endif
> +#ifdef CONFIG_BATMAN_ADV_MCAST
> +	atomic_t multicast_mode;
> +#endif

Kernel doc ?

Cheers,
Marek

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]