Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: Fix TT sync flags for intermediate TT responses

[Sven Eckelmann <sven@narfation.org>]

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

On Samstag, 5. Mai 2018 17:30:20 CEST Linus Lüssing wrote:
[...]
> diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> index 0225616d..a95724ea 100644
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c
> @@ -2914,6 +2914,41 @@ static bool batadv_tt_global_valid(const void *entry_ptr,
>  }
>  
>  /**
> + * batadv_tt_get_flags() - get TT flags for a given entry
> + * @tt_common_entry: the TT entry to get the flags for
> + * @orig: for a TT global entry the specific node to get the flags for
> + *
> + * Return: -ENOENT on error or the TT flags otherwise.
> + */
> +static int
> +batadv_tt_get_flags(struct batadv_tt_common_entry *tt_common_entry,
> +		    struct batadv_orig_node *orig)

Not sure whether it is a good idea to use the return here for the error code 
(< 0) and the flags (__u8/enum batadv_tt_client_flags). I will leave the 
decision here to Antonio.

[...]
>   * batadv_tt_tvlv_generate() - fill the tvlv buff with the tt entries from the
>   *  specified tt hash
>   * @bat_priv: the bat priv with all the soft interface information
> @@ -2934,6 +2969,7 @@ static void batadv_tt_tvlv_generate(struct batadv_priv *bat_priv,
>  	struct batadv_tvlv_tt_change *tt_change;
>  	struct hlist_head *head;
>  	u16 tt_tot, tt_num_entries = 0;
> +	int flags;
>  	u32 i;
>  
>  	tt_tot = batadv_tt_entries(tt_len);
> @@ -2951,8 +2987,12 @@ static void batadv_tt_tvlv_generate(struct batadv_priv *bat_priv,
>  			if ((valid_cb) && (!valid_cb(tt_common_entry, cb_data)))
>  				continue;
>  
> +			flags = batadv_tt_get_flags(tt_common_entry, cb_data);
> +			if (flags < 0)
> +				continue;

The second argument of batadv_tt_get_flags is a little bit tricky here. The 
kernel-doc says that cb_data is "data passed to the filter function as 
argument" and is of type "void *". But you are now using it here as if would 
always be of type "struct batadv_orig_node *". This doesn't make problem  for 
now because only two functions are using this argument - luckily they are 
either set it to NULL or to "batadv_orig_node *".

I would propose to make it clear that this argument must be 
"struct batadv_orig_node *" and not an arbitrary argument which is only used 
by the valid_cb callback.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]