Re: [B.A.T.M.A.N.] [PATCH] batman-adv: change VID semantic in the BLA code

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

Hey Antonio,

On Wed, Apr 03, 2013 at 11:23:59AM +0200, Antonio Quartulli wrote:
> On Wed, Apr 03, 2013 at 11:17:14AM +0200, Antonio Quartulli wrote:
> > From: Antonio Quartulli <antonio@open-mesh.com>
> > 
> > In order to make batman-adv fully vlan aware later, the
> > semantic used for variables storing the VLAN ID values has
> > to be changed in order to be adapted to the new one which
> > will be used batman-adv wide.

That is for the TT change later I guess? Was confused first, because
batman-adv is already pretty VLAN aware ... maybe add this as a comment?

> > 
> > In particular, the VID has to be an "_unsigned_ short int"
> > and its 4 MSB will be used as a flag bitfield, while the
> > remaining 12 bits are used to store the real VID value
> > 
> > Cc: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
> > Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> 
> [cut..]
> 
> > diff --git a/packet.h b/packet.h
> > index a51ccfc..d5464f6 100644
> > --- a/packet.h
> > +++ b/packet.h
> > @@ -105,6 +105,14 @@ enum batadv_tt_client_flags {
> >  	BATADV_TT_CLIENT_PENDING = BIT(10),
> >  };
> >  
> > +/**
> > + * batadv_vlan_flags - flags for the four MSB of any vlan ID field
> > + * @BATADV_VLAN_HAS_TAG: whether the field contains a valid vlan tag or not
> > + */
> > +enum batadv_vlan_flags {
> > +	BATADV_VLAN_HAS_TAG	= BIT(15),
> > +};
> > +

Please put this into main.h or somewhere else as long as it is not sent over
the wire.

> >  /* claim frame types for the bridge loop avoidance */
> >  enum batadv_bla_claimframe {
> >  	BATADV_CLAIM_TYPE_CLAIM		= 0x00,
> > diff --git a/soft-interface.c b/soft-interface.c
> > index 403b8c4..34597a2 100644
> > --- a/soft-interface.c
> > +++ b/soft-interface.c
> > @@ -154,7 +154,7 @@ static int batadv_interface_tx(struct sk_buff *skb,
> >  						    0x00, 0x00};
> >  	unsigned int header_len = 0;
> >  	int data_len = skb->len, ret;
> > -	short vid __maybe_unused = -1;
> > +	unsigned short vid __maybe_unused = BATADV_NO_FLAGS;
> >  	bool do_bcast = false;
> >  	uint32_t seqno;
> >  	unsigned long brd_delay = 1;
> > @@ -303,7 +303,7 @@ void batadv_interface_rx(struct net_device *soft_iface,
> >  	struct ethhdr *ethhdr;
> >  	struct vlan_ethhdr *vhdr;
> >  	struct batadv_header *batadv_header = (struct batadv_header *)skb->data;
> > -	short vid __maybe_unused = -1;
> > +	unsigned short vid __maybe_unused = BATADV_NO_FLAGS;
> >  	__be16 ethertype = __constant_htons(ETH_P_BATMAN);
> >  	bool is_bcast;
> 
> I just realised that this change is going to break compatibility because we
> change the menaing of the value that BLA sends over the wire.
> 
> We must postpone this change to the next (BIG) compat bump.

Actually no, it just uses the VID internally, so this is not a problem. See:
http://www.open-mesh.org/projects/batman-adv/wiki/Bridge-loop-avoidance-Protocol

If there is a VID, it will send the frame in the respective VLAN.

The patch looks fine in generally, I have no objections.

Cheers,
	Simon

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