On Fri, May 24, 2013 at 02:23:41PM +0800, Marek Lindner wrote:
On Friday, May 24, 2013 04:15:21 Antonio Quartulli wrote:
From: Antonio Quartulli antonio@open-mesh.com
Since batman-adv is now fully VLAN-aware, a proper framework able to handle per-vlan-interface attributes is now needed.
We have "now" twice in the same sentence. :)
diff --git a/compat.c b/compat.c index da556a4..b99d505 100644 --- a/compat.c +++ b/compat.c @@ -27,6 +27,15 @@
#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 0, 0)
+void batadv_free_rcu_priv_vlan(struct rcu_head *rcu) +{
- struct batadv_priv_vlan *vlan;
- vlan = container_of(rcu, struct batadv_priv_vlan, rcu);
- kfree(vlan);
+}
I bet you didn't try to compile that on an older kernel ...
ah, no I did not. :) I'll add the prototype to compat.h
+/**
- batadv_get_priv_vlan - get VLAN priv data
The naming could be better. Following your initial scheme this function should be called "batadv_priv_vlan_get". How about replacing "priv" with "softif" ? We'd end up with:
- batadv_softif_vlan_free_ref()
- batadv_softif_vlan_get()
ok, will use 'softif'
+/**
- batadv_add_vid - ndo_add_vid API implementation
Again, the naming .. didn't we introduce a naming convention for each file ? soft-interface.c uses "interface" a lot. Why not using it ? For example, batadv_interface_vid_add() ?
ok, I will add 'interface'.
- @dev: the netdev of the mesh interface
- @vid: identifier of the new VLAN
- Set up all the internal structures for handling the new VLAN on top of
the
- mesh interface
- */
Returns ?
ops :)
+/**
- batadv_kill_vid - ndo_kill_vid API implementation
Same here. It should be batadv_interface_vid_kill() or something similar.
ok
- @dev: the netdev of the mesh interface
- @vid: identifier of the deleted VLAN
- Destroy all the internal structures used to handle the VLAN identified
by vid + * on top of the mesh interface
- */
Returns ?
ops :)
+static int batadv_kill_vid(struct net_device *dev, unsigned short vid) +{
- struct batadv_priv *bat_priv = netdev_priv(dev);
- struct batadv_priv_vlan *vlan, *vlan_tmp = NULL;
- list_for_each_entry_rcu(vlan, &bat_priv->priv_vlan_list, list) {
if (vlan->vid != vid)continue;vlan_tmp = vlan;break;- }
How about re-using your get() function from above ?
I'll check, I remember there was a reason to do not use gat().
+/*
- struct batadv_priv_vlan - per VLAN attributes set
- @vid: VLAN identifier
- @kobj: kobject for sysfs vlan subdirectory
- @list: list node for bat_priv::priv_vlan_list
- @refcount: number of context where this object is currently in use
- @rcu: struct used for freeing in a RCU-safe manner
- */
+struct batadv_priv_vlan {
- unsigned short vid;
- struct kobject *kobj;
- struct list_head list;
- atomic_t refcount;
- struct rcu_head rcu;
+};
I am not so happy about the choice regarding the name again. The "batadv_priv" prefix has been introduced for bat_priv sub structs. Clearly, this isn't one but an element for a list.
ok, I'll change it to softif as stated above.
/**
- struct batadv_priv - per mesh interface data
- @mesh_state: current status of the mesh (inactive/active/deactivating)
@@ -573,6 +589,9 @@ struct batadv_priv_nc {
- @primary_if: one of the hard interfaces assigned to this mesh interface
- becomes the primary interface
- @bat_algo_ops: routing algorithm used by this mesh interface
- @priv_vlan_list: a list of priv_vlan structs, one per VLAN created on
top of + * the mesh interface represented by this object
- @priv_vlan_list_lock: lock protecting priv_vlan_list
- @bla: bridge loope avoidance data
- @debug_log: holding debug logging relevant data
- @gw: gateway data
@@ -620,6 +639,8 @@ struct batadv_priv { struct work_struct cleanup_work; struct batadv_hard_iface __rcu *primary_if; /* rcu protected pointer */ struct batadv_algo_ops *bat_algo_ops;
- struct list_head priv_vlan_list;
- spinlock_t priv_vlan_list_lock; /* protects priv_vlan_list */
#ifdef CONFIG_BATMAN_ADV_BLA struct batadv_priv_bla bla; #endif
This leads to weird code like: bat_priv->priv_vlan_list_lock (priv twice) ..
yeah, softif everywhere! :D
Thanks a lot!
I'll send v3 and I'll include the BLA integration too.
Cheers,