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

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

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.

> +
> +	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?

> +/* 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)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]