[1/3] batman-adv: detect clients connected through a 802.11 device
Commit Message
Clients connected through a 802.11 device are now marked with the
TT_CLIENT_WIFI flag. This flag is also advertised with the tt
announcement.
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
compat.h | 2 +
main.c | 2 +-
packet.h | 3 +-
routing.c | 2 +-
soft-interface.c | 4 +-
translation-table.c | 59 ++++++++++++++++++++++++++++++++++++++++----------
translation-table.h | 17 +++++++++-----
types.h | 2 +-
8 files changed, 67 insertions(+), 24 deletions(-)
Comments
Hi Antonio
>
> - tt_local_add(soft_iface, soft_iface->dev_addr);
> + tt_local_add(soft_iface, soft_iface->dev_addr, 0);
Reading the code, it is not obvious what this 0 mean here. Did you see
Sven's recent patches which introduced enums in various places? Maybe
instead of a bool you could have an enum with two values: wired, wifi?
I've not looked at the other patches yet, but it might even make sense
to have enums for wired, AP, client, adhoc?
> +/* This function check whether the interface represented by ifindex is a
> + * 802.11 wireless device or not. If so the tt_local_entry is marked with the
> + * TT_CLIENT_WIFI flag */
> +static void tt_check_iface(struct tt_local_entry *tt_local_entry, int ifindex)
It would be nice to have a more descriptive name, one which tells you
what it is checking. Here it is clear, because of the comment, but
where it is actually used, it is not possible to know what the
function is checking.
Maybe what is actually needed is a function:
bool is_iface_wifi(int ifindex)
and maybe that should be in hard-interface.c? It could be that knowing
if an interface is wired or wifi could be useful in other places. This
was one of the discussions at WBMv4, giving preference to wired
interfaces over wireless.
Andrew
Hi Andrew, Thank you for replying
On dom, giu 05, 2011 at 11:42:36 +0200, Andrew Lunn wrote:
> Hi Antonio
>
> >
> > - tt_local_add(soft_iface, soft_iface->dev_addr);
> > + tt_local_add(soft_iface, soft_iface->dev_addr, 0);
>
> Reading the code, it is not obvious what this 0 mean here. Did you see
> Sven's recent patches which introduced enums in various places? Maybe
> instead of a bool you could have an enum with two values: wired, wifi?
>
> I've not looked at the other patches yet, but it might even make sense
> to have enums for wired, AP, client, adhoc?
>
Actually I only know whether the device is 802.11 or not. It could also be
something different from both wired and wireless. However you are right,
I should avoid the hardcoded zero. Maybe I can use NO_FLAGS as we did
for the other patches.
Later on someone else could add more enum values (interface types) and avoid
using NO_FLAGS here (TT_CLIENT_WIFI is an enum already).
> > +/* This function check whether the interface represented by ifindex is a
> > + * 802.11 wireless device or not. If so the tt_local_entry is marked with the
> > + * TT_CLIENT_WIFI flag */
> > +static void tt_check_iface(struct tt_local_entry *tt_local_entry, int ifindex)
>
> It would be nice to have a more descriptive name, one which tells you
> what it is checking. Here it is clear, because of the comment, but
> where it is actually used, it is not possible to know what the
> function is checking.
>
> Maybe what is actually needed is a function:
>
> bool is_iface_wifi(int ifindex)
>
sounds good. It is definitely simpler to read and to understand.
> and maybe that should be in hard-interface.c? It could be that knowing
> if an interface is wired or wifi could be useful in other places. This
> was one of the discussions at WBMv4, giving preference to wired
> interfaces over wireless.
>
mh..Yes, once the function is named is_iface_wifi(), it makes definitely
sense to move it in hard-interface.c and make it usable by other part of
the code.
Regards,
Hi all,
On lun, giu 06, 2011 at 12:01:10 +0200, Antonio Quartulli wrote:
> Hi Andrew, Thank you for replying
>
> On dom, giu 05, 2011 at 11:42:36 +0200, Andrew Lunn wrote:
> > Hi Antonio
> >
> > >
> > > - tt_local_add(soft_iface, soft_iface->dev_addr);
> > > + tt_local_add(soft_iface, soft_iface->dev_addr, 0);
> >
> > Reading the code, it is not obvious what this 0 mean here. Did you see
> > Sven's recent patches which introduced enums in various places? Maybe
> > instead of a bool you could have an enum with two values: wired, wifi?
> >
> > I've not looked at the other patches yet, but it might even make sense
> > to have enums for wired, AP, client, adhoc?
> >
>
> Actually I only know whether the device is 802.11 or not. It could also be
> something different from both wired and wireless. However you are right,
> I should avoid the hardcoded zero. Maybe I can use NO_FLAGS as we did
> for the other patches.
Ok, this was not the case for NO_FLAGS (this is not a flag field).
I introduced a new constant:
#define NULL_IFINDEX 0 /* dummy ifindex used to avoid iface checks */
I can't use an enum here because it is just a simple MACRO, not a set of
values. However does it make sense to have several constats defined as
0? should we always use the same? But what about the name?
> Later on someone else could add more enum values (interface types) and avoid
> using NO_FLAGS here (TT_CLIENT_WIFI is an enum already).
It is late. I mixed things up. These are two separated issues:
1) With NULL_IFINDEX I account the hardcoded 0.
2) Later on more enum values similar to TT_CLIENT_WIFI (e.g.
TT_CLIENT_ETH, TT_CLIENT_ADHOC, etc..) can be added. But till now we
only have is_wifi_iface().
Regards,
On Sunday, June 05, 2011 11:01:02 PM Antonio Quartulli wrote:
> +/* This function returns true if the interface represented by ifindex is a
> + * 802.11 wireless device */
> +bool is_wifi_iface(int ifindex)
> +{
> + struct net_device *net_device;
> +
> + if (ifindex == NULL_IFINDEX)
> + return false;
> +
> + net_device = dev_get_by_index(&init_net, ifindex);
> + if (!net_device)
> + return false;
> +
> +#ifdef CONFIG_WIRELESS_EXT
> + /* pre-cfg80211 drivers have to implement WEXT, so it is possible to
> + * check for wireless_handlers != NULL */
> + if (net_device->wireless_handlers)
> + return true;
> + else
> +#endif
> + /* cfg80211 drivers have to set ieee80211_ptr */
> + if (net_device->ieee80211_ptr)
> + return true;
> + return false;
> +}
If I am not mistaken dev_get_by_index() increases a counter on the returned
interface. We have to decrease that counter as soon as we don't need the
interface anymore otherwise the interface can never be freed.
> @@ -108,7 +108,7 @@ int mesh_init(struct net_device *soft_iface)
> if (tt_init(bat_priv) < 1)
> goto err;
>
> - tt_local_add(soft_iface, soft_iface->dev_addr);
> + tt_local_add(soft_iface, soft_iface->dev_addr, NULL_IFINDEX);
Are you sure 0 is not a valid index for any interface ?
> static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
> - const uint8_t *addr, bool roaming)
> + const uint8_t *addr, bool roaming, bool wifi)
How about adding a set of flags (TT_CLIENT_ROAM / TT_CLIENT_WIFI / etc)
instead of adding more and more bool arguments ? In several places the code
converts one to the other which does not seem necessary.
> +void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> + int ifindex)
> {
> struct bat_priv *bat_priv = netdev_priv(soft_iface);
> struct tt_local_entry *tt_local_entry = NULL;
> @@ -204,21 +231,22 @@ void tt_local_add(struct net_device *soft_iface,
> const uint8_t *addr) if (!tt_local_entry)
> goto out;
>
> - tt_local_event(bat_priv, NO_FLAGS, addr, false);
> -
> bat_dbg(DBG_TT, bat_priv,
> "Creating new local tt entry: %pM (ttvn: %d)\n", addr,
> (uint8_t)atomic_read(&bat_priv->ttvn));
>
> memcpy(tt_local_entry->addr, addr, ETH_ALEN);
> tt_local_entry->last_seen = jiffies;
> + tt_local_entry->flags = 0;
Here you should make use of the NO_FLAGS define you introduced. :-)
> /* the batman interface mac address should never be purged */
> if (compare_eth(addr, soft_iface->dev_addr))
> - tt_local_entry->never_purge = 1;
> - else
> - tt_local_entry->never_purge = 0;
> + tt_local_entry->flags |= TT_LOCAL_NOPURGE;
> +
> + tt_local_event(bat_priv, NO_FLAGS, addr, false,
> + tt_local_entry->flags & TT_CLIENT_WIFI);
Changing "never_purge" to a flag probably is a good idea but should go into a
separate patch.
Regards,
Marek
On mer, giu 15, 2011 at 11:28:28 +0200, Marek Lindner wrote:
> On Sunday, June 05, 2011 11:01:02 PM Antonio Quartulli wrote:
> > +/* This function returns true if the interface represented by ifindex is a
> > + * 802.11 wireless device */
> > +bool is_wifi_iface(int ifindex)
> > +{
> > + struct net_device *net_device;
> > +
> > + if (ifindex == NULL_IFINDEX)
> > + return false;
> > +
> > + net_device = dev_get_by_index(&init_net, ifindex);
> > + if (!net_device)
> > + return false;
> > +
> > +#ifdef CONFIG_WIRELESS_EXT
> > + /* pre-cfg80211 drivers have to implement WEXT, so it is possible to
> > + * check for wireless_handlers != NULL */
> > + if (net_device->wireless_handlers)
> > + return true;
> > + else
> > +#endif
> > + /* cfg80211 drivers have to set ieee80211_ptr */
> > + if (net_device->ieee80211_ptr)
> > + return true;
> > + return false;
> > +}
>
> If I am not mistaken dev_get_by_index() increases a counter on the returned
> interface. We have to decrease that counter as soon as we don't need the
> interface anymore otherwise the interface can never be freed.
Definitely right!
>
>
> > @@ -108,7 +108,7 @@ int mesh_init(struct net_device *soft_iface)
> > if (tt_init(bat_priv) < 1)
> > goto err;
> >
> > - tt_local_add(soft_iface, soft_iface->dev_addr);
> > + tt_local_add(soft_iface, soft_iface->dev_addr, NULL_IFINDEX);
>
> Are you sure 0 is not a valid index for any interface ?
Yes. You can also check the function dev_new_index() at
http://lxr.linux.no/linux+v2.6.39/net/core/dev.c#L5080
indexes start from 1 :)
>
>
> > static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
> > - const uint8_t *addr, bool roaming)
> > + const uint8_t *addr, bool roaming, bool wifi)
>
> How about adding a set of flags (TT_CLIENT_ROAM / TT_CLIENT_WIFI / etc)
> instead of adding more and more bool arguments ? In several places the code
> converts one to the other which does not seem necessary.
You mean simply passing a int value which is combination of the used
flags? mught be a good idea, even for further changes.
>
>
> > +void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
> > + int ifindex)
> > {
> > struct bat_priv *bat_priv = netdev_priv(soft_iface);
> > struct tt_local_entry *tt_local_entry = NULL;
> > @@ -204,21 +231,22 @@ void tt_local_add(struct net_device *soft_iface,
> > const uint8_t *addr) if (!tt_local_entry)
> > goto out;
> >
> > - tt_local_event(bat_priv, NO_FLAGS, addr, false);
> > -
> > bat_dbg(DBG_TT, bat_priv,
> > "Creating new local tt entry: %pM (ttvn: %d)\n", addr,
> > (uint8_t)atomic_read(&bat_priv->ttvn));
> >
> > memcpy(tt_local_entry->addr, addr, ETH_ALEN);
> > tt_local_entry->last_seen = jiffies;
> > + tt_local_entry->flags = 0;
>
> Here you should make use of the NO_FLAGS define you introduced. :-)
Right :)
>
>
> > /* the batman interface mac address should never be purged */
> > if (compare_eth(addr, soft_iface->dev_addr))
> > - tt_local_entry->never_purge = 1;
> > - else
> > - tt_local_entry->never_purge = 0;
> > + tt_local_entry->flags |= TT_LOCAL_NOPURGE;
> > +
> > + tt_local_event(bat_priv, NO_FLAGS, addr, false,
> > + tt_local_entry->flags & TT_CLIENT_WIFI);
>
> Changing "never_purge" to a flag probably is a good idea but should go into a
> separate patch.
Ok, I'll propose a separated patch.
Thank you for reviewing!
Regards,
On Thursday, June 16, 2011 12:43:55 AM Antonio Quartulli wrote:
> > > - tt_local_add(soft_iface, soft_iface->dev_addr);
> > > + tt_local_add(soft_iface, soft_iface->dev_addr, NULL_IFINDEX);
> >
> > Are you sure 0 is not a valid index for any interface ?
>
> Yes. You can also check the function dev_new_index() at
> http://lxr.linux.no/linux+v2.6.39/net/core/dev.c#L5080
> indexes start from 1 :)
Ok.
> > How about adding a set of flags (TT_CLIENT_ROAM / TT_CLIENT_WIFI / etc)
> > instead of adding more and more bool arguments ? In several places the
> > code converts one to the other which does not seem necessary.
>
> You mean simply passing a int value which is combination of the used
> flags? mught be a good idea, even for further changes.
Yes, that is what I had in mind.
Regards,
Marek
@@ -263,6 +263,8 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
#define __always_unused __attribute__((unused))
+#define skb_iif iif
+
#endif /* < KERNEL_VERSION(2, 6, 33) */
#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 34)
@@ -108,7 +108,7 @@ int mesh_init(struct net_device *soft_iface)
if (tt_init(bat_priv) < 1)
goto err;
- tt_local_add(soft_iface, soft_iface->dev_addr);
+ tt_local_add(soft_iface, soft_iface->dev_addr, 0);
if (vis_init(bat_priv) < 1)
goto err;
@@ -75,7 +75,8 @@ enum tt_query_flags {
/* TT_CHANGE flags */
enum tt_change_flags {
TT_CHANGE_DEL = 0x01,
- TT_CLIENT_ROAM = 0x02
+ TT_CLIENT_ROAM = 0x02,
+ TT_CLIENT_WIFI = 0x04
};
struct batman_packet {
@@ -1290,7 +1290,7 @@ int recv_roam_adv(struct sk_buff *skb, struct hard_iface *recv_if)
roam_adv_packet->client);
tt_global_add(bat_priv, orig_node, roam_adv_packet->client,
- atomic_read(&orig_node->last_ttvn) + 1, true);
+ atomic_read(&orig_node->last_ttvn) + 1, true, false);
/* Roaming phase starts: I have new information but the ttvn has not
* been incremented yet. This flag will make me check all the incoming
@@ -535,7 +535,7 @@ static int interface_set_mac_addr(struct net_device *dev, void *p)
if (atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE) {
tt_local_remove(bat_priv, dev->dev_addr,
"mac address changed", false);
- tt_local_add(dev, addr->sa_data);
+ tt_local_add(dev, addr->sa_data, 0);
}
memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
@@ -593,7 +593,7 @@ int interface_tx(struct sk_buff *skb, struct net_device *soft_iface)
goto dropped;
/* Register the client MAC in the transtable */
- tt_local_add(soft_iface, ethhdr->h_source);
+ tt_local_add(soft_iface, ethhdr->h_source, skb->skb_iif);
if (is_multicast_ether_addr(ethhdr->h_dest)) {
ret = gw_is_target(bat_priv, skb);
@@ -144,7 +144,7 @@ static void tt_global_entry_free_ref(struct tt_global_entry *tt_global_entry)
}
static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
- const uint8_t *addr, bool roaming)
+ const uint8_t *addr, bool roaming, bool wifi)
{
struct tt_change_node *tt_change_node;
@@ -158,6 +158,9 @@ static void tt_local_event(struct bat_priv *bat_priv, uint8_t op,
if (roaming)
tt_change_node->change.flags |= TT_CLIENT_ROAM;
+ if (wifi)
+ tt_change_node->change.flags |= TT_CLIENT_WIFI;
+
memcpy(tt_change_node->change.addr, addr, ETH_ALEN);
spin_lock_bh(&bat_priv->tt_changes_list_lock);
@@ -187,7 +190,31 @@ static int tt_local_init(struct bat_priv *bat_priv)
return 1;
}
-void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
+/* This function check whether the interface represented by ifindex is a
+ * 802.11 wireless device or not. If so the tt_local_entry is marked with the
+ * TT_CLIENT_WIFI flag */
+static void tt_check_iface(struct tt_local_entry *tt_local_entry, int ifindex)
+{
+ struct net_device *net_device;
+
+ net_device = dev_get_by_index(&init_net, ifindex);
+ if (!net_device)
+ return;
+
+#ifdef CONFIG_WIRELESS_EXT
+ /* pre-cfg80211 driver have to implement WEXT, so it is possible to
+ * check for wireless_handlers != NULL */
+ if (net_device->wireless_handlers)
+ tt_local_entry->flags |= TT_CLIENT_WIFI;
+ else
+#endif
+ /* cfg80211 drivers have to set ieee80211_ptr */
+ if (net_device->ieee80211_ptr)
+ tt_local_entry->flags |= TT_CLIENT_WIFI;
+}
+
+void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
+ int ifindex)
{
struct bat_priv *bat_priv = netdev_priv(soft_iface);
struct tt_local_entry *tt_local_entry = NULL;
@@ -204,21 +231,22 @@ void tt_local_add(struct net_device *soft_iface, const uint8_t *addr)
if (!tt_local_entry)
goto out;
- tt_local_event(bat_priv, NO_FLAGS, addr, false);
-
bat_dbg(DBG_TT, bat_priv,
"Creating new local tt entry: %pM (ttvn: %d)\n", addr,
(uint8_t)atomic_read(&bat_priv->ttvn));
memcpy(tt_local_entry->addr, addr, ETH_ALEN);
tt_local_entry->last_seen = jiffies;
+ tt_local_entry->flags = 0;
+ tt_check_iface(tt_local_entry, ifindex);
atomic_set(&tt_local_entry->refcount, 2);
/* the batman interface mac address should never be purged */
if (compare_eth(addr, soft_iface->dev_addr))
- tt_local_entry->never_purge = 1;
- else
- tt_local_entry->never_purge = 0;
+ tt_local_entry->flags |= TT_LOCAL_NOPURGE;
+
+ tt_local_event(bat_priv, NO_FLAGS, addr, false,
+ tt_local_entry->flags & TT_CLIENT_WIFI);
hash_add(bat_priv->tt_local_hash, compare_ltt, choose_orig,
tt_local_entry, &tt_local_entry->hash_entry);
@@ -388,7 +416,8 @@ void tt_local_remove(struct bat_priv *bat_priv, const uint8_t *addr,
if (!tt_local_entry)
goto out;
- tt_local_event(bat_priv, TT_CHANGE_DEL, tt_local_entry->addr, roaming);
+ tt_local_event(bat_priv, TT_CHANGE_DEL, tt_local_entry->addr, roaming,
+ false);
tt_local_del(bat_priv, tt_local_entry, message);
out:
if (tt_local_entry)
@@ -411,7 +440,7 @@ static void tt_local_purge(struct bat_priv *bat_priv)
spin_lock_bh(list_lock);
hlist_for_each_entry_safe(tt_local_entry, node, node_tmp,
head, hash_entry) {
- if (tt_local_entry->never_purge)
+ if (tt_local_entry->flags & TT_LOCAL_NOPURGE)
continue;
if (!is_out_of_time(tt_local_entry->last_seen,
@@ -419,7 +448,7 @@ static void tt_local_purge(struct bat_priv *bat_priv)
continue;
tt_local_event(bat_priv, TT_CHANGE_DEL,
- tt_local_entry->addr, false);
+ tt_local_entry->addr, false, false);
atomic_dec(&bat_priv->num_local_tt);
bat_dbg(DBG_TT, bat_priv, "Deleting local "
"tt entry (%pM): timed out\n",
@@ -495,7 +524,8 @@ static void tt_changes_list_free(struct bat_priv *bat_priv)
/* caller must hold orig_node refcount */
int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
- const unsigned char *tt_addr, uint8_t ttvn, bool roaming)
+ const unsigned char *tt_addr, uint8_t ttvn, bool roaming,
+ bool wifi)
{
struct tt_global_entry *tt_global_entry;
struct orig_node *orig_node_tmp;
@@ -537,6 +567,9 @@ int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
tt_global_entry->roam_at = 0;
}
+ if (wifi)
+ tt_global_entry->flags |= TT_CLIENT_WIFI;
+
bat_dbg(DBG_TT, bat_priv,
"Creating new global tt entry: %pM (via %pM)\n",
tt_global_entry->addr, orig_node->orig);
@@ -1342,7 +1375,9 @@ static void _tt_update_changes(struct bat_priv *bat_priv,
(tt_change + i)->flags & TT_CLIENT_ROAM);
else
if (!tt_global_add(bat_priv, orig_node,
- (tt_change + i)->addr, ttvn, false))
+ (tt_change + i)->addr, ttvn, false,
+ (tt_change + i)->flags &
+ TT_CLIENT_WIFI))
/* In case of problem while storing a
* global_entry, we stop the updating
* procedure without committing the
@@ -22,20 +22,25 @@
#ifndef _NET_BATMAN_ADV_TRANSLATION_TABLE_H_
#define _NET_BATMAN_ADV_TRANSLATION_TABLE_H_
+/* Transtable local entry flags */
+enum tt_local_flags {
+ TT_LOCAL_NOPURGE = 0x01
+};
+
int tt_len(int changes_num);
int tt_changes_fill_buffer(struct bat_priv *bat_priv,
unsigned char *buff, int buff_len);
int tt_init(struct bat_priv *bat_priv);
-void tt_local_add(struct net_device *soft_iface, const uint8_t *addr);
+void tt_local_add(struct net_device *soft_iface, const uint8_t *addr,
+ int ifindex);
void tt_local_remove(struct bat_priv *bat_priv,
const uint8_t *addr, const char *message, bool roaming);
int tt_local_seq_print_text(struct seq_file *seq, void *offset);
-void tt_global_add_orig(struct bat_priv *bat_priv,
- struct orig_node *orig_node,
+void tt_global_add_orig(struct bat_priv *bat_priv, struct orig_node *orig_node,
const unsigned char *tt_buff, int tt_buff_len);
-int tt_global_add(struct bat_priv *bat_priv,
- struct orig_node *orig_node, const unsigned char *addr,
- uint8_t ttvn, bool roaming);
+int tt_global_add(struct bat_priv *bat_priv, struct orig_node *orig_node,
+ const unsigned char *addr, uint8_t ttvn, bool roaming,
+ bool wifi);
int tt_global_seq_print_text(struct seq_file *seq, void *offset);
void tt_global_del_orig(struct bat_priv *bat_priv,
struct orig_node *orig_node, const char *message);
@@ -223,7 +223,7 @@ struct socket_packet {
struct tt_local_entry {
uint8_t addr[ETH_ALEN];
unsigned long last_seen;
- char never_purge;
+ uint8_t flags;
atomic_t refcount;
struct rcu_head rcu;
struct hlist_node hash_entry;