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
