| Message ID | 1441111438-30921-2-git-send-email-dedeckeh@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 2015-09-01 14:43, Hans Dedecker wrote: > The function set_state disable is not called for external devices in device_release > which means for external vlan/macvlan devices they won't be deleted. > As a result of this the set_state enable call for external devices by device_claim fails > as vlan/macvlan devices cannot be created since the device already exists in the kernel. > Therefore move the external device check from device_set_state to device_claim so > external vlan/macvlan devices are not created again and can also be external. Why/how do vlan/macvlan devices become external? - Felix
On Tue, Sep 1, 2015 at 2:49 PM, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-09-01 14:43, Hans Dedecker wrote: > > The function set_state disable is not called for external devices in > device_release > > which means for external vlan/macvlan devices they won't be deleted. > > As a result of this the set_state enable call for external devices by > device_claim fails > > as vlan/macvlan devices cannot be created since the device already > exists in the kernel. > > Therefore move the external device check from device_set_state to > device_claim so > > external vlan/macvlan devices are not created again and can also be > external. > Why/how do vlan/macvlan devices become external? > This use case is driven by an external application which is adding a vlan device to the bridge. Initially the vlan device is created as an internal device but not added to the bridge; later added by an external application via ubus to the bridge. In this scenario we hit the issue when doing network reload the vlan device is not anymore an active member of the bridge as vlan set_state enable was called which failed. This is a bit similar to a wireless device which has uci device config; initially it's considered as an internal device by netifd when loading the config but becomes an external device when the wireless logic adds it to the bridge. Hans > > - Felix >
On 2015-09-01 15:53, Hans Dedecker wrote: > > > On Tue, Sep 1, 2015 at 2:49 PM, Felix Fietkau <nbd@openwrt.org > <mailto:nbd@openwrt.org>> wrote: > > On 2015-09-01 14:43, Hans Dedecker wrote: > > The function set_state disable is not called for external devices > in device_release > > which means for external vlan/macvlan devices they won't be deleted. > > As a result of this the set_state enable call for external devices > by device_claim fails > > as vlan/macvlan devices cannot be created since the device already > exists in the kernel. > > Therefore move the external device check from device_set_state to > device_claim so > > external vlan/macvlan devices are not created again and can also > be external. > Why/how do vlan/macvlan devices become external? > > This use case is driven by an external application which is adding a > vlan device to the bridge. > Initially the vlan device is created as an internal device but not added > to the bridge; later added by an external application via ubus to the > bridge. In this scenario we hit the issue when doing network reload the > vlan device is not anymore an active member of the bridge as vlan > set_state enable was called which failed. > This is a bit similar to a wireless device which has uci device config; > initially it's considered as an internal device by netifd when loading > the config but becomes an external device when the wireless logic adds > it to the bridge. The main point of dev->external is to declare that a device is fully managed externally, and netifd is not supposed to bring it up or down. Maybe a better fix would be to allow devices to be added dynamically without forcing dev->external on them (or just prevent that flag from being added for vlan devices). - Felix
On Wed, Sep 2, 2015 at 10:31 AM, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-09-01 15:53, Hans Dedecker wrote: > > > > > > On Tue, Sep 1, 2015 at 2:49 PM, Felix Fietkau <nbd@openwrt.org > > <mailto:nbd@openwrt.org>> wrote: > > > > On 2015-09-01 14:43, Hans Dedecker wrote: > > > The function set_state disable is not called for external devices > > in device_release > > > which means for external vlan/macvlan devices they won't be > deleted. > > > As a result of this the set_state enable call for external devices > > by device_claim fails > > > as vlan/macvlan devices cannot be created since the device already > > exists in the kernel. > > > Therefore move the external device check from device_set_state to > > device_claim so > > > external vlan/macvlan devices are not created again and can also > > be external. > > Why/how do vlan/macvlan devices become external? > > > > This use case is driven by an external application which is adding a > > vlan device to the bridge. > > Initially the vlan device is created as an internal device but not added > > to the bridge; later added by an external application via ubus to the > > bridge. In this scenario we hit the issue when doing network reload the > > vlan device is not anymore an active member of the bridge as vlan > > set_state enable was called which failed. > > This is a bit similar to a wireless device which has uci device config; > > initially it's considered as an internal device by netifd when loading > > the config but becomes an external device when the wireless logic adds > > it to the bridge. > The main point of dev->external is to declare that a device is fully > managed externally, and netifd is not supposed to bring it up or down. > Maybe a better fix would be to allow devices to be added dynamically > without forcing dev->external on them (or just prevent that flag from > being added for vlan devices). > Devices can be added dynamically without being forced external via the function interface_handle_link; so that is covered. Regarding the external flag being set for vlan devices; would it be acceptable the external flag can only be set for simple devices in device_get ? This is the only place where non simple devices can be set external. My intention with this patch was also to line up the external flag check when the state is altered; in device_release the external flag is checked although the same check is done in set_device_state (which would eventually be called by the set_state callback function). So I thought to add the same external flag check in device_claim to line up both functions. Hans > > - Felix >
On 2015-09-02 13:27, Hans Dedecker wrote: > > > On Wed, Sep 2, 2015 at 10:31 AM, Felix Fietkau <nbd@openwrt.org > <mailto:nbd@openwrt.org>> wrote: > > On 2015-09-01 15:53, Hans Dedecker wrote: > > > > > > On Tue, Sep 1, 2015 at 2:49 PM, Felix Fietkau <nbd@openwrt.org > <mailto:nbd@openwrt.org> > > <mailto:nbd@openwrt.org <mailto:nbd@openwrt.org>>> wrote: > > > > On 2015-09-01 14:43, Hans Dedecker wrote: > > > The function set_state disable is not called for external > devices > > in device_release > > > which means for external vlan/macvlan devices they won't be > deleted. > > > As a result of this the set_state enable call for external > devices > > by device_claim fails > > > as vlan/macvlan devices cannot be created since the device > already > > exists in the kernel. > > > Therefore move the external device check from > device_set_state to > > device_claim so > > > external vlan/macvlan devices are not created again and can also > > be external. > > Why/how do vlan/macvlan devices become external? > > > > This use case is driven by an external application which is adding a > > vlan device to the bridge. > > Initially the vlan device is created as an internal device but not > added > > to the bridge; later added by an external application via ubus to the > > bridge. In this scenario we hit the issue when doing network > reload the > > vlan device is not anymore an active member of the bridge as vlan > > set_state enable was called which failed. > > This is a bit similar to a wireless device which has uci device > config; > > initially it's considered as an internal device by netifd when loading > > the config but becomes an external device when the wireless logic adds > > it to the bridge. > The main point of dev->external is to declare that a device is fully > managed externally, and netifd is not supposed to bring it up or down. > Maybe a better fix would be to allow devices to be added dynamically > without forcing dev->external on them (or just prevent that flag from > being added for vlan devices). > > Devices can be added dynamically without being forced external via the > function interface_handle_link; so that is covered. > Regarding the external flag being set for vlan devices; would it be > acceptable the external flag can only be set for simple devices in > device_get ? This is the only place where non simple devices can be set > external. > My intention with this patch was also to line up the external flag check > when the state is altered; in device_release the external flag is > checked although the same check is done in set_device_state (which would > eventually be called by the set_state callback function). So I thought > to add the same external flag check in device_claim to line up both > functions. Okay, seems I didn't look at the context closely enough. I think this patch makes sense after all, and I will apply it. - Felix
diff --git a/device.c b/device.c index 0d73138..21b436f 100644 --- a/device.c +++ b/device.c @@ -81,9 +81,6 @@ static int set_device_state(struct device *dev, bool state) return -1; } - if (dev->external) - return 0; - if (state) system_if_up(dev); else @@ -324,7 +321,7 @@ void device_broadcast_event(struct device *dev, enum device_event ev) int device_claim(struct device_user *dep) { struct device *dev = dep->dev; - int ret; + int ret = 0; if (dep->claimed) return 0; @@ -335,7 +332,9 @@ int device_claim(struct device_user *dep) return 0; device_broadcast_event(dev, DEV_EVENT_SETUP); - ret = dev->set_state(dev, true); + if (!dev->external) + ret = dev->set_state(dev, true); + if (ret == 0) device_broadcast_event(dev, DEV_EVENT_UP); else {
The function set_state disable is not called for external devices in device_release which means for external vlan/macvlan devices they won't be deleted. As a result of this the set_state enable call for external devices by device_claim fails as vlan/macvlan devices cannot be created since the device already exists in the kernel. Therefore move the external device check from device_set_state to device_claim so external vlan/macvlan devices are not created again and can also be external. Signed-off-by: Hans Dedecker <dedeckeh@gmail.com> --- device.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)