Re: [B.A.T.M.A.N.] [PATCHv6 2/3] batman-adv: Adding 'mcast' log level

Sorry, it seems the quotation is a little messed up, but I hope you can understand the rest:

This patch adds an 'mcast' log level. Currently, it will print changes relevant to a nodes own multicast flag changes.

Signed-off-by: Linus Lüssing linus.luessing@web.de

multicast.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- soft-interface.c | 5 ++ types.h | 17 ++++++ 3 files changed, 169 insertions(+), 5 deletions(-)

diff --git a/multicast.c b/multicast.c index c05f64a..2a80997 100644 --- a/multicast.c +++ b/multicast.c @@ -292,6 +292,123 @@ static bool batadv_mcast_has_bridge(struct batadv_priv *bat_priv) }

/**

• • batadv_mcast_querier_log - debug output regarding the querier status on

link + * @bat_priv: the bat priv with all the soft interface information +

• @str_proto: a string for the querier protocol (e.g. "IGMP" or "MLD") + *

@old_querier: the previous querier state on our link

• • @new_querier: the new querier state on our link

I think changing the variable name from old/new_querier to old/new_state would make this part more understandable - there are not two different queriers after all, just their state changes.

• • Outputs debug messages to the logging facility with log level 'mcast'
• • regarding changes to the querier status on the link which are relevant
• • to our multicast optimizations.
• • Usually this is about whether a querier appeared or vanished in
• • our mesh or whether the querier is in the suboptimal position of being
• • behind our local bridge segment.

Maybe it would be good to briefly explain at that point again why it's suboptimal?

• • This is only interesting for nodes with a bridge on top of their
• • soft interface.
• */

+static void +batadv_mcast_querier_log(struct batadv_priv *bat_priv, char *str_proto,

• 	 struct batadv_mcast_querier_state *old_querier,
  

• 	 struct batadv_mcast_querier_state *new_querier)
  

+{

• if (!old_querier->exists && new_querier->exists)

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,
  

• 	   "%s Querier appeared (good!)\n", str_proto);
  

• else if (old_querier->exists && !new_querier->exists)

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,
  

• 	   "%s Querier disappeared (bad)\n", str_proto);
  

• else if (!bat_priv->mcast.bridged && !new_querier->exists)

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,
  

• 	   "Note: No %s Querier present\n", str_proto);
  

• if (!new_querier->exists)

• return;
  

• if (!old_querier->shadowing && new_querier->shadowing)

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,
  

• 	   "%s Querier is behind our bridged segment: Might shadow 
  

listeners

(bad)\n", + str_proto);

• else if (old_querier->shadowing && !new_querier->shadowing)

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,
  

• 	   "%s Querier is not behind our bridged segment (good!)\n",
  

• 	   str_proto);
  

I'm not sure if good/bad is really useful to write here - at least we don't really tell why its good or bad?

[...] +/**

• • batadv_mcast_flags_logs - output debug information about mcast flag

changes + * @bat_priv: the bat priv with all the soft interface information + * @mcast_flags: flags indicating the new multicast state

• • Whenever the multicast flags this nodes announces changes (@mcast_flags

vs. + * bat_priv->mcast.flags), this notifies userspace via the 'mcast' log level. + */ +static void batadv_mcast_flags_log(struct batadv_priv *bat_priv, uint8_t flags) +{

• char str_undefined[] = "<undefined>";
• char str_flags[] = "[...]";
• char *str_flags_old = str_undefined;

Can't you write str_flags_old = "<undefined>" directly? why that indirection?

• uint8_t old_flags = bat_priv->mcast.flags;
• if (!bat_priv->mcast.enabled)

• goto print;
  

• str_flags_old = str_flags;
• sprintf(str_flags_old, "[%c%c%c]",

• old_flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES ? 'U' : '.',
  

• old_flags & BATADV_MCAST_WANT_ALL_IPV4 ? '4' : '.',
  

• old_flags & BATADV_MCAST_WANT_ALL_IPV6 ? '6' : '.');
  

Ah, you want to reuse this buffer? hmm ...

+print:

• batadv_dbg(BATADV_DBG_MCAST, bat_priv,

•    "Changing multicast flags from '%s' to '[%c%c%c]'\n",
  

•    str_flags_old,
  

•    flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES ? 'U' : '.',
  

•    flags & BATADV_MCAST_WANT_ALL_IPV4 ? '4' : '.',
  

•    flags & BATADV_MCAST_WANT_ALL_IPV6 ? '6' : '.');
  

I think this overly complex to do.

How about always writing str_flags_old accoding to old flags, but then do:

batadv_dbg(BATADV_DBG_MCAST, bat_priv, "Changing multicast flags from '%s' to '[%c%c%c]'\n", bat_priv->mcast.enabled ? str_flags_old : "<undefined>", flags & BATADV_MCAST_WANT_ALL_UNSNOOPABLES ? 'U' : '.', flags & BATADV_MCAST_WANT_ALL_IPV4 ? '4' : '.', flags & BATADV_MCAST_WANT_ALL_IPV6 ? '6' : '.');

+}

+/**

• batadv_mcast_mla_tvlv_update - update multicast tvlv
• @bat_priv: the bat priv with all the soft interface information

@@ -304,18 +421,33 @@ static bool batadv_mcast_has_bridge(struct batadv_priv *bat_priv) static bool batadv_mcast_mla_tvlv_update(struct batadv_priv *bat_priv) { struct batadv_tvlv_mcast_data mcast_data;

• struct batadv_mcast_querier_state querier_ipv4, querier_ipv6; struct net_device *dev = bat_priv->soft_iface;

• bool bridged;

  mcast_data.flags = BATADV_NO_FLAGS; memset(mcast_data.reserved, 0, sizeof(mcast_data.reserved));

• memset(&querier_ipv4, 0, sizeof(querier_ipv4));

• memset(&querier_ipv6, 0, sizeof(querier_ipv6));

since it just has two elements, you could do that above in the initialization:

struct batadv_mcast_querier_state querier_ipv4 = {false, false}; struct batadv_mcast_querier_state querier_ipv6 = {false, false};

• if (!batadv_mcast_has_bridge(bat_priv))

• bridged = batadv_mcast_has_bridge(bat_priv);
• if (!bridged) goto update;

#if !IS_ENABLED(CONFIG_BRIDGE_IGMP_SNOOPING) pr_warn_once("No bridge IGMP snooping compiled - multicast optimizations disabled\n"); #endif

• querier_ipv4 = (struct batadv_mcast_querier_state){

• .exists = br_multicast_has_querier_anywhere(dev, ETH_P_IP),
  

• .shadowing = br_multicast_has_querier_adjacent(dev, ETH_P_IP),
  

• };
• querier_ipv6 = (struct batadv_mcast_querier_state){

• .exists = br_multicast_has_querier_anywhere(dev, ETH_P_IPV6),
  

• .shadowing = br_multicast_has_querier_adjacent(dev, ETH_P_IPV6),
  

• };

That would be shorter and more readable by just writing:

querier_ipv4.exists = br_multicast_has_querier_anywhere(dev, ETH_P_IP); querier_ipv4.shadowing = br_multicast_has_querier_adjacent(dev, ETH_P_IP);

querier_ipv6.exists = br_multicast_has_querier_anywhere(dev, ETH_P_IPV6); querier_ipv6.shadowing = br_multicast_has_querier_adjacent(dev, ETH_P_IPV6);

mcast_data.flags |= BATADV_MCAST_WANT_ALL_UNSNOOPABLES;

/* 1) If no querier exists at all, then multicast listeners on @@ -327,21 +459,31 @@ static bool batadv_mcast_mla_tvlv_update(struct batadv_priv *bat_priv) * In both cases, we will signalize other batman nodes that * we need all multicast traffic of the according protocol. */

• if (!br_multicast_has_querier_anywhere(dev, ETH_P_IP) ||

•    br_multicast_has_querier_adjacent(dev, ETH_P_IP))
  

• if (!querier_ipv4.exists || querier_ipv4.shadowing) mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV4;

• if (!br_multicast_has_querier_anywhere(dev, ETH_P_IPV6) ||

•    br_multicast_has_querier_adjacent(dev, ETH_P_IPV6))
  

• if (!querier_ipv6.exists || querier_ipv6.shadowing) mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV6;

update:

• batadv_mcast_bridge_log(bat_priv, bridged, &querier_ipv4,

• 		&querier_ipv6);
  

• if (memcmp(&bat_priv->mcast.querier_ipv4, &querier_ipv4,

•    sizeof(querier_ipv4)))
  

• bat_priv->mcast.querier_ipv4 = querier_ipv4;
  

• if (memcmp(&bat_priv->mcast.querier_ipv6, &querier_ipv6,

•    sizeof(querier_ipv6)))
  

• bat_priv->mcast.querier_ipv6 = querier_ipv6;
  

Why comparing and then assigning? You could assign always instead, that should eventually not cost more than this construction.

if (!bat_priv->mcast.enabled || mcast_data.flags != bat_priv->mcast.flags) {

• batadv_mcast_flags_log(bat_priv, mcast_data.flags);
  

  batadv_tvlv_container_register(bat_priv, BATADV_TVLV_MCAST, 1, &mcast_data, sizeof(mcast_data)); bat_priv->mcast.flags = mcast_data.flags; bat_priv->mcast.enabled = true;

• bat_priv->mcast.bridged = bridged;
  

  }

  return !(mcast_data.flags &

diff --git a/soft-interface.c b/soft-interface.c index 9bf382d..0265a58 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -745,7 +745,12 @@ static int batadv_softif_init_late(struct net_device *dev) atomic_set(&bat_priv->distributed_arp_table, 1); #endif #ifdef CONFIG_BATMAN_ADV_MCAST

• bat_priv->mcast.querier_ipv4 = (struct batadv_mcast_querier_state){

• .exists = false, .shadowing = false };
  

• bat_priv->mcast.querier_ipv6 = (struct batadv_mcast_querier_state){

• .exists = false, .shadowing = false };
  

yet again, just do:

bat_priv->mcast.querier_ipv4.exists = false; bat_priv->mcast.querier_ipv4.shadowing = false; bat_priv->mcast.querier_ipv6.exists = false; bat_priv->mcast.querier_ipv6.shadowing = false;

[...] Cheers, Simon