diff mbox

[OpenWrt-Devel,netifd,1/3] device: Fix device find failure in avl list due to device name change

Message ID 1465207423-20413-1-git-send-email-dedeckeh@gmail.com
State Changes Requested
Headers show

Commit Message

Hans Dedecker June 6, 2016, 10:03 a.m. UTC 
As device name is used as key in avl list a device name change will break the avl find logic.
Function device_set_ifname offers api to set the device name and re-inserts the avl node in the list
when the avl key value is changed.

Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
---
 alias.c  |  9 +++------
 device.c | 27 ++++++++++++++++++++++++---
 device.h |  1 +
 vlan.c   | 11 +++++++----
 4 files changed, 35 insertions(+), 13 deletions(-)

Comments

Eyal Birger June 6, 2016, 10:38 a.m. UTC | #1 
Hi Hans,

On Mon, Jun 6, 2016 at 1:03 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
> As device name is used as key in avl list a device name change will break the avl find logic.
> Function device_set_ifname offers api to set the device name and re-inserts the avl node in the list
> when the avl key value is changed.
>
> Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
> ---
>
> diff --git a/device.c b/device.c
> index 06c4820..7149d86 100644
> --- a/device.c
> +++ b/device.c
> @@ -422,14 +422,14 @@ void device_init_virtual(struct device *dev, const struct device_type *type, con
>         assert(dev);
>         assert(type);
>
> -       if (name)
> -               strncpy(dev->ifname, name, IFNAMSIZ);
> -
>         D(DEVICE, "Initialize device '%s'\n", dev->ifname);

Isn't this debug print broken by this change?

>         INIT_SAFE_LIST(&dev->users);
>         INIT_SAFE_LIST(&dev->aliases);
>         dev->type = type;
>
> +       if (name)
> +               device_set_ifname(dev, name);
> +


Eyal.
Hans Dedecker June 6, 2016, 11:06 a.m. UTC | #2 
On Mon, Jun 6, 2016 at 12:38 PM, Eyal Birger <eyal.birger@gmail.com> wrote:
> Hi Hans,
>
> On Mon, Jun 6, 2016 at 1:03 PM, Hans Dedecker <dedeckeh@gmail.com> wrote:
>> As device name is used as key in avl list a device name change will break the avl find logic.
>> Function device_set_ifname offers api to set the device name and re-inserts the avl node in the list
>> when the avl key value is changed.
>>
>> Signed-off-by: Hans Dedecker <dedeckeh@gmail.com>
>> ---
>>
>> diff --git a/device.c b/device.c
>> index 06c4820..7149d86 100644
>> --- a/device.c
>> +++ b/device.c
>> @@ -422,14 +422,14 @@ void device_init_virtual(struct device *dev, const struct device_type *type, con
>>         assert(dev);
>>         assert(type);
>>
>> -       if (name)
>> -               strncpy(dev->ifname, name, IFNAMSIZ);
>> -
>>         D(DEVICE, "Initialize device '%s'\n", dev->ifname);
>
> Isn't this debug print broken by this change?
>
>>         INIT_SAFE_LIST(&dev->users);
>>         INIT_SAFE_LIST(&dev->aliases);
>>         dev->type = type;
>>
>> +       if (name)
>> +               device_set_ifname(dev, name);
>> +
>
>
> Eyal.
Correct I will send a V2

Thx,
Hans
diff mbox

Patch

diff --git a/alias.c b/alias.c
index e4bb700..6b938ca 100644
--- a/alias.c
+++ b/alias.c
@@ -59,13 +59,10 @@  static void alias_set_device(struct alias_device *alias, struct device *dev)
 	alias->dev.hidden = !dev;
 	if (dev) {
 		device_set_ifindex(&alias->dev, dev->ifindex);
-		strcpy(alias->dev.ifname, dev->ifname);
-		device_broadcast_event(&alias->dev, DEV_EVENT_UPDATE_IFNAME);
+		device_set_ifname(&alias->dev, dev->ifname);
 		device_add_user(&alias->dep, dev);
-	} else {
-		alias->dev.ifname[0] = 0;
-		device_broadcast_event(&alias->dev, DEV_EVENT_UPDATE_IFNAME);
-	}
+	} else
+		device_set_ifname(&alias->dev, "");
 }
 
 static int
diff --git a/device.c b/device.c
index 06c4820..7149d86 100644
--- a/device.c
+++ b/device.c
@@ -422,14 +422,14 @@  void device_init_virtual(struct device *dev, const struct device_type *type, con
 	assert(dev);
 	assert(type);
 
-	if (name)
-		strncpy(dev->ifname, name, IFNAMSIZ);
-
 	D(DEVICE, "Initialize device '%s'\n", dev->ifname);
 	INIT_SAFE_LIST(&dev->users);
 	INIT_SAFE_LIST(&dev->aliases);
 	dev->type = type;
 
+	if (name)
+		device_set_ifname(dev, name);
+
 	if (!dev->set_state)
 		dev->set_state = set_device_state;
 }
@@ -582,6 +582,27 @@  void device_set_ifindex(struct device *dev, int ifindex)
 	device_broadcast_event(dev, DEV_EVENT_UPDATE_IFINDEX);
 }
 
+int device_set_ifname(struct device *dev, const char *name)
+{
+	int ret = 0;
+
+	if (!strcmp(dev->ifname, name))
+		return 0;
+
+	if (dev->avl.key)
+		avl_delete(&devices, &dev->avl);
+
+	strncpy(dev->ifname, name, IFNAMSIZ);
+
+	if (dev->avl.key)
+		ret = avl_insert(&devices, &dev->avl);
+
+	if (ret == 0)
+		device_broadcast_event(dev, DEV_EVENT_UPDATE_IFNAME);
+
+	return ret;
+}
+
 static int device_refcount(struct device *dev)
 {
 	struct list_head *list;
diff --git a/device.h b/device.h
index 77a2fef..b069a2b 100644
--- a/device.h
+++ b/device.h
@@ -250,6 +250,7 @@  void device_broadcast_event(struct device *dev, enum device_event ev);
 void device_set_present(struct device *dev, bool state);
 void device_set_link(struct device *dev, bool state);
 void device_set_ifindex(struct device *dev, int ifindex);
+int device_set_ifname(struct device *dev, const char *name);
 void device_refresh_present(struct device *dev);
 int device_claim(struct device_user *dep);
 void device_release(struct device_user *dep);
diff --git a/vlan.c b/vlan.c
index ac434ce..7f8697b 100644
--- a/vlan.c
+++ b/vlan.c
@@ -63,8 +63,11 @@  static int vlan_set_device_state(struct device *dev, bool up)
 
 static void vlan_dev_set_name(struct vlan_device *vldev, struct device *dev)
 {
+	char name[IFNAMSIZ];
+
 	vldev->dev.hidden = dev->hidden;
-	snprintf(vldev->dev.ifname, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
+	snprintf(name, IFNAMSIZ, "%s.%d", dev->ifname, vldev->id);
+	device_set_ifname(&vldev->dev, name);
 }
 
 static void vlan_dev_cb(struct device_user *dep, enum device_event ev)
@@ -81,7 +84,6 @@  static void vlan_dev_cb(struct device_user *dep, enum device_event ev)
 		break;
 	case DEV_EVENT_UPDATE_IFNAME:
 		vlan_dev_set_name(vldev, dep->dev);
-		device_broadcast_event(&vldev->dev, ev);
 		break;
 	case DEV_EVENT_TOPO_CHANGE:
 		/* Propagate topo changes */
@@ -124,9 +126,10 @@  static struct device *get_vlan_device(struct device *dev, int id, bool create)
 		return NULL;
 
 	vldev->id = id;
-	vlan_dev_set_name(vldev, dev);
 
-	device_init(&vldev->dev, &vlan_type, vldev->dev.ifname);
+	device_init(&vldev->dev, &vlan_type, NULL);
+
+	vlan_dev_set_name(vldev, dev);
 	vldev->dev.default_config = true;
 
 	vldev->set_state = vldev->dev.set_state;