Re: [B.A.T.M.A.N.] [PATCHv6 1/3] batman-adv: Multicast Listener Announcements via Translation Table

[Marek Lindner <lindner_marek@yahoo.de>]

On Saturday, June 15, 2013 01:50:07 Linus Lüssing wrote:
> With this patch a node which has no bridge interface on top of its soft
> interface announces its local multicast listeners via the translation
> table.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
>  Makefile               |    2 +
>  Makefile.kbuild        |    1 +
>  compat.h               |   20 ++++-
>  gen-compat-autoconf.sh |    1 +
>  main.c                 |    6 ++
>  main.h                 |    1 +
>  multicast.c            |  210
> ++++++++++++++++++++++++++++++++++++++++++++++++ multicast.h            | 
>  43 ++++++++++
>  soft-interface.c       |    3 +
>  sysfs.c                |    6 ++
>  translation-table.c    |   22 ++++-
>  types.h                |   12 +++
>  12 files changed, 322 insertions(+), 5 deletions(-)
>  create mode 100644 multicast.c
>  create mode 100644 multicast.h
> 
> diff --git a/Makefile b/Makefile
> index 407cdc4..d7c6fa6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -27,6 +27,8 @@ export CONFIG_BATMAN_ADV_BLA=y
>  export CONFIG_BATMAN_ADV_DAT=y
>  # B.A.T.M.A.N network coding (catwoman):
>  export CONFIG_BATMAN_ADV_NC=n
> +# B.A.T.M.A.N. multicast optimizations:
> +export CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS=y

Can we please find a shorter define ? How about "CONFIG_BATMAN_ADV_MCAST" ? 
That would be in line with the rest.


> +struct batadv_hw_addr {
> +	struct list_head	list;
> +	unsigned char addr[ETH_ALEN];
> +};

All struct defines should go into types.h or packet.h if they are sent over 
the wire.


> +/**
> + * batadv_mcast_mla_tt_update - update the own MLAs
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Update the own multicast listener announcements in the translation
> + * table. Also take 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 net_device *soft_iface = bat_priv->soft_iface;
> +	struct list_head mcast_list;
> +	int ret;
> +	static bool enabled;
> +
> +	INIT_LIST_HEAD(&mcast_list);
> +
> +	/* Avoid attaching MLAs, if multicast optimization is disabled
> +	 * or there is a bridge on top of our soft interface (TODO)
> +	 */
> +	if (!atomic_read(&bat_priv->mcast_group_awareness) ||
> +	    bat_priv->soft_iface->priv_flags & IFF_BRIDGE_PORT) {
> +		if (enabled)
> +			enabled = false;
> +
> +		goto update;
> +	}
> +
> +	if (!enabled)
> +		enabled = true;
> +
> +	ret = batadv_mcast_mla_local_collect(soft_iface, &mcast_list);
> +	if (ret < 0)
> +		goto out;
> +
> +update:
> +	batadv_mcast_mla_tt_clean(bat_priv, &mcast_list);
> +	batadv_mcast_mla_tt_add(bat_priv, &mcast_list);
> +
> +out:
> +	batadv_mcast_mla_collect_free(&mcast_list);
> +}

You can use the LIST_HEAD static initializer.
The "enabled" variable does not make any sense.

How about we change some function names for the sake of clarity ? Here my 
suggestions:

 * batadv_mcast_mla_local_collect() => batadv_mcast_mla_softif_retrieve() or 
batadv_mcast_mla_softif_get()
 * batadv_mcast_mla_tt_clean() => batadv_mcast_mla_tt_retract()
 * batadv_mcast_mla_collect_free() => batadv_mcast_mla_list_free()


> +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
> +struct batadv_priv_mcast {
> +	struct list_head mla_list;
> +};
> +#endif

I don't see a good reason to use a double linked list or is there one ?

Cheers,
Marek