Hi Simon, thanks for reviewing this patch.
On Sun, Apr 07, 2013 at 04:20:53PM +0200, Simon Wunderlich wrote:
> 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 <anto...@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?
>
well, the fact that it works does not make it aware, no? :)
Actually batman-adv does not take any action and does not recognise the event of
creating a new VLAN on top of it.
But from your comment I realise that "being aware" is probably not the correct
term. So I will try to improve it.
> > >
> > > 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 <s...@hrz.tu-chemnitz.de>
> > > Signed-off-by: Antonio Quartulli <anto...@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.
Ok. I put it there because I'll be sending it over the wire later. But better
putting it into main.h for now.
>
> > > /* 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.
>
My bad, I looked at the patch stats and I misinterpreted _my own_ change to
packet.h :)
> The patch looks fine in generally, I have no objections.
>
Thanks a lot! I'll send v2.
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
pgplhcBGGPYt_.pgp
Description: PGP signature
