Re: [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: Announce new capability via multicast TVLV

["Linus Lüssing" <linus.luessing@web.de>]

On Sun, May 12, 2013 at 01:11:13AM +0200, Antonio Quartulli wrote:
> On Sat, May 11, 2013 at 07:23:26PM +0200, Linus Lüssing wrote:
> >  /**
> > + * 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
> > + * @flags: flags indicating the tvlv state (see batadv_tvlv_handler_flags)
> > + * @tvlv_value: tvlv buffer containing the multicast data
> > + * @tvlv_value_len: tvlv buffer length
> > + */
> > +static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
> > +					     struct batadv_orig_node *orig,
> > +					     uint8_t flags,
> > +					     void *tvlv_value,
> > +					     uint16_t tvlv_value_len)
> > +{
> > +	uint8_t mcast_flags = BATADV_NO_FLAGS;
> > +
> > +	/* only fetch the tvlv value if the handler wasn't called via the
> > +	 * CIFNOTFND flag and if there is data to fetch
> > +	 */
> > +	if (!(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND) &&
> > +	    (tvlv_value) && (tvlv_value_len == 1))
> 		just for style reason I'd suggest to use sizeof(mcast_flags)
> 		instead of 1. and there is no need for parentheses around
> 		tvlv_value.
> 
> > +		mcast_flags = *(unsigned char *)tvlv_value;
> 
> mcast_flags is uint8_t, therefore (even if it may practically be the same) you
> should use the same type for the cast.

ok

> 
> > +
> > +	if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) &&
> > +	    orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) {
> > +		atomic_inc(&bat_priv->mcast_num_non_aware);
> > +	} else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT &&
> > +		   !(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) {
> > +		atomic_dec(&bat_priv->mcast_num_non_aware);
> > +	}
> > +
> > +	orig->mcast_flags = mcast_flags;
> > +}
> 
> > diff --git a/originator.c b/originator.c
> > index 5d53d2f..acc0c2d 100644
> > --- a/originator.c
> > +++ b/originator.c
> > @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
> >  	reset_time = jiffies - 1 - msecs_to_jiffies(BATADV_RESET_PROTECTION_MS);
> >  	orig_node->bcast_seqno_reset = reset_time;
> >  	orig_node->batman_seqno_reset = reset_time;
> > +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
> > +	orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
> > +#endif
> 
> why do you start assuming that an originator has the optimisation enabled? would
> it be better to wait for the first mcast tvlv from it to claim this?

Because it is easier code-wise. I had it the other way round first
and issuing an atomic_inc(&bat_priv->mcast_num_non_ware) in
batadv_get_orig_node(), but then I ended up counting up too many
times because I was increasing that counter for secondary
interface originators, too while not decreasing it again because
no TVLV handler will be called for these. And within
batadv_get_orig_node() I don't see an easy way to determine
whether I was called for a primary or secondary interface
originator.

> 
> > +/* multicast capabilities */
> > +enum batadv_mcast_flags {
> > +	BATADV_MCAST_LISTENER_ANNOUNCEMENT = BIT(0),
> > +};
> > +
> 
> > diff --git a/types.h b/types.h
> > index 5d73a75..4ef5fb9 100644
> > --- a/types.h
> > +++ b/types.h
> > @@ -146,6 +146,9 @@ struct batadv_orig_node {
> >  	unsigned long last_seen;
> >  	unsigned long bcast_seqno_reset;
> >  	unsigned long batman_seqno_reset;
> > +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
> > +	uint8_t mcast_flags;
> > +#endif
> >  	uint8_t capabilities;
> >  	atomic_t last_ttvn;
> >  	uint32_t tt_crc;
> > @@ -569,6 +572,7 @@ struct batadv_priv {
> >  #endif
> >  #ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
> >  	atomic_t mcast_group_awareness;
> > +	atomic_t mcast_num_non_aware;
> 
> why isn't this variable in the mcast_priv struct? It is not a user knob (as far
> as I can see)

You're right, will move it.

> 
> Cheers,
> 
> -- 
> Antonio Quartulli
> 
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara


Cheers, Linus