[1/2] batman-adv: move and restructure batadv_v_ogm_forward

  From: Simon Wunderlich <simon@open-mesh.com>

To match our code better to the protocol description of B.A.T.M.A.N. V,
move batadv_v_ogm_forward() out into batadv_v_ogm_process_per_outif()
and move all checks directly deciding whether the OGM should be
forwarded into batadv_v_ogm_forward().

Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
---
 net/batman-adv/bat_v_ogm.c | 102 +++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 45 deletions(-)
  

On 20/01/16 21:32, Simon Wunderlich wrote:
>  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
>  				 const struct batadv_ogm2_packet *ogm_received,
> -				 u32 throughput,
> +				 struct batadv_neigh_node *neigh_node,
>  				 struct batadv_hard_iface *if_incoming,
>  				 struct batadv_hard_iface *if_outgoing)
>  {
> +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> +	struct batadv_neigh_node *router = NULL;
>  	struct batadv_ogm2_packet *ogm_forward;
>  	unsigned char *skb_buff;
>  	struct sk_buff *skb;
>  	size_t packet_len;
>  	u16 tvlv_len;
>  
> +	/* only forward for specific interfaces, not for the default one. */
> +	if (if_outgoing != BATADV_IF_DEFAULT)
> +		goto out;
> +

personally I'd prefer the forward function to do what it says: forward
the OGM. The checks whether or not we should do this should stay in
ogm_process_per_outif().

> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> +	if (!orig_ifinfo)
> +		goto out;
> +	/* acquire possibly updated router */
> +	router = batadv_orig_router_get(orig_node, if_outgoing);
> +
> +	/* strict rule: forward packets coming from the best next hop only */
> +	if (neigh_node != router)
> +		goto out;
> +

this is changing the behaviour.
here now we get a router which potentially was elected during the
previous update_route() call while processing this very OGM. We are
still discussing if we want to do this or not, but this patch should be
just a style change, while this is not.

This check should be performed against the router which was selected
before performing any route update.


Cheers,
  

On Wednesday 20 January 2016 22:34:25 Antonio Quartulli wrote:
> On 20/01/16 21:32, Simon Wunderlich wrote:
> >  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
> >  
> >  				 const struct batadv_ogm2_packet *ogm_received,
> > 
> > -				 u32 throughput,
> > +				 struct batadv_neigh_node *neigh_node,
> > 
> >  				 struct batadv_hard_iface *if_incoming,
> >  				 struct batadv_hard_iface *if_outgoing)
> >  
> >  {
> > 
> > +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> > +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> > +	struct batadv_neigh_node *router = NULL;
> > 
> >  	struct batadv_ogm2_packet *ogm_forward;
> >  	unsigned char *skb_buff;
> >  	struct sk_buff *skb;
> >  	size_t packet_len;
> >  	u16 tvlv_len;
> > 
> > +	/* only forward for specific interfaces, not for the default one. */
> > +	if (if_outgoing != BATADV_IF_DEFAULT)
> > +		goto out;
> > +
> 
> personally I'd prefer the forward function to do what it says: forward
> the OGM. The checks whether or not we should do this should stay in
> ogm_process_per_outif().
> 

Hm, yeah we discussed to keep these changes in route_update, but actually 
those checks decide whether we forward or not (i.e. directly involving 
forwarding). Therefore I thought they would fit better in the forward function 
and also move the corresponding description [1] in the OGMv2 wiki page from 
route update to forward. We don't have a matching "general" description in the 
OGMv2 wiki page which corresponds to ogm_process_per_outif() after all.

Any other opinions?

[1] From the OGMv2 wiki page: If the OGMv2 has been received by the (now) 
selected router, the OGM is forwarded on the considered outgoing interface 
(except for the default interface). However, the OGMv2 is not forwarded if 
another OGMv2 has been forwarded with the same sequence number.
> > +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);
> > +	if (!orig_ifinfo)
> > +		goto out;
> > +	/* acquire possibly updated router */
> > +	router = batadv_orig_router_get(orig_node, if_outgoing);
> > +
> > +	/* strict rule: forward packets coming from the best next hop only */
> > +	if (neigh_node != router)
> > +		goto out;
> > +
> 
> this is changing the behaviour.
> here now we get a router which potentially was elected during the
> previous update_route() call while processing this very OGM. We are
> still discussing if we want to do this or not, but this patch should be
> just a style change, while this is not.

No, this is already in the code which is merged into master - we already 
acquire the updated router (see bat_v_ogm.c:547, function 
batadv_v_ogm_route_update()).

Cheers,
    Simon
  

On 20/01/16 23:18, Simon Wunderlich wrote:
>> this is changing the behaviour.
>> here now we get a router which potentially was elected during the
>> previous update_route() call while processing this very OGM. We are
>> still discussing if we want to do this or not, but this patch should be
>> just a style change, while this is not.
> 
> No, this is already in the code which is merged into master - we already 
> acquire the updated router (see bat_v_ogm.c:547, function 
> batadv_v_ogm_route_update()).

uhuhuh?! Actually you are right!
This means we currently send one OGM every time we make an election,
thus we might send multiple OGMs with the sequence numnber, despite this
is still under debate.

As far as I remember did not want to follow this approach at the moment?
Am I missing something?


Cheers,
  

On 20/01/16 23:26, Antonio Quartulli wrote:
> 
> 
> On 20/01/16 23:18, Simon Wunderlich wrote:
>>> this is changing the behaviour.
>>> here now we get a router which potentially was elected during the
>>> previous update_route() call while processing this very OGM. We are
>>> still discussing if we want to do this or not, but this patch should be
>>> just a style change, while this is not.
>>
>> No, this is already in the code which is merged into master - we already 
>> acquire the updated router (see bat_v_ogm.c:547, function 
>> batadv_v_ogm_route_update()).
> 
> uhuhuh?! Actually you are right!
> This means we currently send one OGM every time we make an election,
> thus we might send multiple OGMs with the sequence numnber, despite this
> is still under debate.
> 
> As far as I remember did not want to follow this approach at the moment?
> Am I missing something?
> 

I was missing this:

+       /* don't forward the same seqno twice on one interface */
+       if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
+               goto out;
+

thanks Marek for pointing this out for me :)

it's all good then!

Cheers,
  

On Wednesday 20 January 2016 23:26:15 Antonio Quartulli wrote:
> On 20/01/16 23:18, Simon Wunderlich wrote:
> >> this is changing the behaviour.
> >> here now we get a router which potentially was elected during the
> >> previous update_route() call while processing this very OGM. We are
> >> still discussing if we want to do this or not, but this patch should be
> >> just a style change, while this is not.
> > 
> > No, this is already in the code which is merged into master - we already
> > acquire the updated router (see bat_v_ogm.c:547, function
> > batadv_v_ogm_route_update()).
> 
> uhuhuh?! Actually you are right!
> This means we currently send one OGM every time we make an election,
> thus we might send multiple OGMs with the sequence numnber, despite this
> is still under debate.

No, we only send once per sequence number, see below.
> 
> As far as I remember did not want to follow this approach at the moment?
> Am I missing something?

Right now we store the last sent sequence number for an originator in 
orig_ifinfo->last_seqno_forwarded when forwarding. This field also gets checked 
and the OGMv2 gets dropped before forwarding when receive it again.

So yes, we send immediately after we make a new election, but only send an OGM 
once per sequence number.

Cheers,
     Simon
  

On Wednesday 20 January 2016 23:29:21 Antonio Quartulli wrote:
> On 20/01/16 23:26, Antonio Quartulli wrote:
> > On 20/01/16 23:18, Simon Wunderlich wrote:
> >>> this is changing the behaviour.
> >>> here now we get a router which potentially was elected during the
> >>> previous update_route() call while processing this very OGM. We are
> >>> still discussing if we want to do this or not, but this patch should be
> >>> just a style change, while this is not.
> >> 
> >> No, this is already in the code which is merged into master - we already
> >> acquire the updated router (see bat_v_ogm.c:547, function
> >> batadv_v_ogm_route_update()).
> > 
> > uhuhuh?! Actually you are right!
> > This means we currently send one OGM every time we make an election,
> > thus we might send multiple OGMs with the sequence numnber, despite this
> > is still under debate.
> > 
> > As far as I remember did not want to follow this approach at the moment?
> > Am I missing something?
> 
> I was missing this:
> 
> +       /* don't forward the same seqno twice on one interface */
> +       if (orig_ifinfo->last_seqno_forwarded == ntohl(ogm2->seqno))
> +               goto out;
> +
> 
> thanks Marek for pointing this out for me :)
> 
> it's all good then!

Ok cool! I guess we just need a conclusion where we put the conditions, then. 
:)

Cheers,
    Simon
  

On Wednesday, January 20, 2016 14:32:14 Simon Wunderlich wrote:
>  /**
> - * batadv_v_ogm_forward - forward an OGM to the given outgoing interface
> + * batadv_v_ogm_forward - check conditions and forward an OGM to the given
> + *  outgoing interface
>   * @bat_priv: the bat priv with all the soft interface information
>   * @ogm_received: previously received OGM to be forwarded
> - * @throughput: throughput to announce, may vary per outgoing interface
> + * @neigh_node: the neigh_node through with the OGM has been received
>   * @if_incoming: the interface on which this OGM was received on
>   * @if_outgoing: the interface to which the OGM has to be forwarded to
>   *
> @@ -359,28 +360,55 @@ static u32 batadv_v_forward_penalty(struct batadv_priv
> *bat_priv, */
>  static void batadv_v_ogm_forward(struct batadv_priv *bat_priv,
>  				 const struct batadv_ogm2_packet *ogm_received,
> -				 u32 throughput,
> +				 struct batadv_neigh_node *neigh_node,
>  				 struct batadv_hard_iface *if_incoming,
>  				 struct batadv_hard_iface *if_outgoing)
>  {
> +	struct batadv_neigh_ifinfo *neigh_ifinfo = NULL;
> +	struct batadv_orig_ifinfo *orig_ifinfo = NULL;
> +	struct batadv_neigh_node *router = NULL;
>  	struct batadv_ogm2_packet *ogm_forward;
>  	unsigned char *skb_buff;
>  	struct sk_buff *skb;
>  	size_t packet_len;
>  	u16 tvlv_len;
> 
> +	/* only forward for specific interfaces, not for the default one. */
> +	if (if_outgoing != BATADV_IF_DEFAULT)
> +		goto out;
> +
> +	orig_ifinfo = batadv_orig_ifinfo_new(orig_node, if_outgoing);

The variable 'orig_node' isn't defined anywhere in the context of this 
function. It was passed as an argument to batadv_v_ogm_route_update().

Please recompile with 'make CONFIG_BATMAN_ADV_BATMAN_V=y' and fix all errors 
and warnings.  :-)


> +	if (!orig_ifinfo)
> +		goto out;
> +	/* acquire possibly updated router */
> +	router = batadv_orig_router_get(orig_node, if_outgoing);

A new line between the goto and the comment would be nice to have.

Cheers,
Marek