Re: [B.A.T.M.A.N.] [PATCH] batman-adv: limit local translation table max size

[Marek Lindner <lindner_marek@yahoo.de>]

On Sunday, September 01, 2013 02:38:29 Antonio Quartulli wrote:
> > diff --git a/hard-interface.c b/hard-interface.c
> > index c5f871f..c60d3ed 100644
> > --- a/hard-interface.c
> > +++ b/hard-interface.c
> > @@ -266,16 +266,9 @@ static void batadv_check_known_mac_addr(const struct
> > net_device *net_dev)
> > 
> >  int batadv_hardif_min_mtu(struct net_device *soft_iface)
> >  {
> > 
> > -	const struct batadv_priv *bat_priv = netdev_priv(soft_iface);
> > +	struct batadv_priv *bat_priv = netdev_priv(soft_iface);
> 
> Why are you removing the const qualifier?

  CC [M]  /home/marek/batman-adv/hard-interface.o
/home/marek/batman-adv/hard-interface.c: In function ‘batadv_hardif_min_mtu’:
/home/marek/batman-adv/hard-interface.c:286:2: warning: passing argument 1 of 
‘atomic_set’ discards ‘const’ qualifier from pointer target type [enabled by 
default]
/usr/src/linux-headers-3.2.0-4-common/arch/x86/include/asm/atomic.h:35:20: 
note: expected ‘struct atomic_t *’ but argument is of type ‘const struct 
atomic_t *’
/home/marek/batman-adv/hard-interface.c:298:2: warning: passing argument 1 of 
‘atomic_set’ discards ‘const’ qualifier from pointer target type [enabled by 
default]
/usr/src/linux-headers-3.2.0-4-common/arch/x86/include/asm/atomic.h:35:20: 
note: expected ‘struct atomic_t *’ but argument is of type ‘const struct 
atomic_t *’

	
> >  	/* Register the client MAC in the transtable */
> > 
> > -	if (!is_multicast_ether_addr(ethhdr->h_source))
> > -		batadv_tt_local_add(soft_iface, ethhdr->h_source, vid,
> > -				    skb->skb_iif);
> > +	if (!is_multicast_ether_addr(ethhdr->h_source)) {
> > +		client_added = batadv_tt_local_add(soft_iface, ethhdr->h_source,
> > +						   vid, skb->skb_iif);
> 
> Isn't this assignment as ugly as the following if condition?
> 
> 		if (!batadv_tt_local_add(soft_iface, ethhdr->h_source, vid,
> 					 skb->skb_iif))
> 
> client_added looks useless to me.

The general idea behind "client_added" was to increase readibility. For the 
casual reader this variable makes it more obvious what goes on.


> > +/**
> > + * batadv_tt_local_table_transmit_size - calculates the local
> > translation table + *  size when transmitted over the air
> > + * @bat_priv: the bat priv with all the soft interface information
> > + *
> > + * Returns local translation table size in bytes.
> > + */
> > +static int batadv_tt_local_table_transmit_size(struct batadv_priv
> > *bat_priv) +{
> > +	struct batadv_softif_vlan *vlan;
> > +	uint16_t num_vlan = 0, tt_local_entries = 0;
> > +	int hdr_size;
> 
> David always asks to sort variable declaration lines from the longest to
> the shorter.
> IMHO it is not really important (in particular in this case) but since you
> are creating the function now, please follow that indication.

Sure.


> > @@ -717,12 +762,6 @@ static void batadv_tt_tvlv_container_update(struct
> > batadv_priv *bat_priv)
> > 
> >  	tt_diff_entries_num = atomic_read(&bat_priv->tt.local_changes);
> >  	tt_diff_len = batadv_tt_len(tt_diff_entries_num);
> > 
> > -	/* if we have too many changes for one packet don't send any
> > -	 * and wait for the tt table request which will be fragmented
> > -	 */
> > -	if (tt_diff_len > bat_priv->soft_iface->mtu)
> > -		tt_diff_len = 0;
> > -
> 
> Why are you removing this? We are not going to fragment OGMs and in this
> function we are preparing the TVLV to send within one of those.

Upon reading this function a realized this "tt_diff_len = 0" mechanism is 
buggy. For instance, the changes_list list is not purged but keeps growing 
until it magically fits the ogm diff again. Or checking bat_priv->soft_iface-
>mtu isn't terribly accurate either. The more I looked the more my head 
started to spin. Suddenly, I was convinced we don't need the check anymore 
because we have the fragmentation but you are right - it won't help here. I'll 
add the check again but be aware that it is broken.

Thanks for the review!

Cheers,
Marek