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

Marek Lindner
2013-06-24T21:47:20Z

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