batman-adv: check return value for hash_add()

  if hash_add() fails, we should remove the structure to avoid memory
leaks.

Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
---
As this was the last unchecked occurence, this should close bug #136
in the open-mesh.org redmine bugtracker.
---
 translation-table.c |   22 ++++++++++++++++++----
 1 files changed, 18 insertions(+), 4 deletions(-)
  

Hello Simon,

On Wed, Nov 02, 2011 at 10:42:49 +0100, Simon Wunderlich wrote:
> if hash_add() fails, we should remove the structure to avoid memory
> leaks.
> 
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> ---
> As this was the last unchecked occurence, this should close bug #136
> in the open-mesh.org redmine bugtracker.
> ---
>  translation-table.c |   22 ++++++++++++++++++----
>  1 files changed, 18 insertions(+), 4 deletions(-)
> 
> @@ -217,16 +218,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> -	hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> +	hash_added = hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
>  		 &tt_local_entry->common, &tt_local_entry->common.hash_entry);

In my opinion you should indent this invocation a bit better :-P

>  
> +	if (unlikely(!hash_added)) {
> +		/* remove the reference for the hash */
> +		tt_local_entry_free_ref(tt_local_entry);
> +		goto out;
> +	}
> +
> +	tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
> +

Here you are invoking tt_local_event() _after_ setting the TT_CLIENT_NEW. This
means that this flag is going to be copied in the tt_change structure and later
it will be sent out within the next OGM. This has to not happen. Therefore I would
move the or-assignment of the TT_CLIENT_NEW flag _after_ the invocation of
tt_local_event().


>  	/* remove address from global hash if present */
>  	tt_global_entry = tt_global_hash_find(bat_priv, addr);
>  
> @@ -499,6 +506,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>  	struct tt_global_entry *tt_global_entry;
>  	struct orig_node *orig_node_tmp;
>  	int ret = 0;
> +	int hash_added;
>  
>  	tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
>  
> @@ -518,9 +526,15 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
>  		tt_global_entry->ttvn = ttvn;
>  		tt_global_entry->roam_at = 0;
>  
> -		hash_add(bat_priv->tt_global_hash, compare_tt,
> +		hash_added = hash_add(bat_priv->tt_global_hash, compare_tt,
>  			 choose_orig, &tt_global_entry->common,
>  			 &tt_global_entry->common.hash_entry);
> +
> +		if (unlikely(!hash_added)) {
> +			/* remove the reference for the hash */
> +			tt_global_entry_free_ref(tt_global_entry);
> +			goto out;
> +		}

In this case the global client could be a roaming one....what about invoking
tt_local_remove() first? Otherwise we would still have a local client which is
actually not one of ours..What do you think? Moreover it would free some space


Cheers,
  

On Wed, Nov 02, 2011 at 11:13:57AM +0100, Antonio Quartulli wrote:
> Hello Simon,
> 
> On Wed, Nov 02, 2011 at 10:42:49 +0100, Simon Wunderlich wrote:
> > if hash_add() fails, we should remove the structure to avoid memory
> > leaks.
> > 
> > Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> > ---
> > As this was the last unchecked occurence, this should close bug #136
> > in the open-mesh.org redmine bugtracker.
> > ---
> >  translation-table.c |   22 ++++++++++++++++++----
> >  1 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > @@ -217,16 +218,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> > -	hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> > +	hash_added = hash_add(bat_priv->tt_local_hash, compare_tt, choose_orig,
> >  		 &tt_local_entry->common, &tt_local_entry->common.hash_entry);
> 
> In my opinion you should indent this invocation a bit better :-P
> 

OK

> >  
> > +	if (unlikely(!hash_added)) {
> > +		/* remove the reference for the hash */
> > +		tt_local_entry_free_ref(tt_local_entry);
> > +		goto out;
> > +	}
> > +
> > +	tt_local_event(bat_priv, addr, tt_local_entry->common.flags);
> > +
> 
> Here you are invoking tt_local_event() _after_ setting the TT_CLIENT_NEW. This
> means that this flag is going to be copied in the tt_change structure and later
> it will be sent out within the next OGM. This has to not happen. Therefore I would
> move the or-assignment of the TT_CLIENT_NEW flag _after_ the invocation of
> tt_local_event().
> 
> 

OK, i will move it.
> >  	/* remove address from global hash if present */
> >  	tt_global_entry = tt_global_hash_find(bat_priv, addr);
> >  
> > @@ -499,6 +506,7 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> >  	struct tt_global_entry *tt_global_entry;
> >  	struct orig_node *orig_node_tmp;
> >  	int ret = 0;
> > +	int hash_added;
> >  
> >  	tt_global_entry = tt_global_hash_find(bat_priv, tt_addr);
> >  
> > @@ -518,9 +526,15 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
> >  		tt_global_entry->ttvn = ttvn;
> >  		tt_global_entry->roam_at = 0;
> >  
> > -		hash_add(bat_priv->tt_global_hash, compare_tt,
> > +		hash_added = hash_add(bat_priv->tt_global_hash, compare_tt,
> >  			 choose_orig, &tt_global_entry->common,
> >  			 &tt_global_entry->common.hash_entry);
> > +
> > +		if (unlikely(!hash_added)) {
> > +			/* remove the reference for the hash */
> > +			tt_global_entry_free_ref(tt_global_entry);
> > +			goto out;
> > +		}
> 
> In this case the global client could be a roaming one....what about invoking
> tt_local_remove() first? Otherwise we would still have a local client which is
> actually not one of ours..What do you think? Moreover it would free some space

Sure, we can do that.

I'll send a v2 patch in a minute ...

thanks
	Simon