| Message ID | 1497615003-24762-1-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello! On 6/16/2017 3:10 PM, Serhey Popovych wrote: > Now with commit 9c7dafb (net: Allow to create links with The commit citing style is standardized: 12-digit SHA1 and the summary enclosed in (""). > given ifindex) support registration of network devices Support for. > with specific ifindex is added. Was added. > We can force loopback network device index before call to > register_netdev() to ensure we always configure it with > LOOPBACK_IFINDEX. > > Kill BUG_ON() since system can continue without network > namespace failed in loopback init path, unless it is > init_net namespace where we panic() anyway. > > Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> [...] MBR, Sergei
From: Serhey Popovych <serhe.popovych@gmail.com> Date: Fri, 16 Jun 2017 15:10:03 +0300 > Now with commit 9c7dafb (net: Allow to create links with > given ifindex) support registration of network devices > with specific ifindex is added. > > We can force loopback network device index before call to > register_netdev() to ensure we always configure it with > LOOPBACK_IFINDEX. > > Kill BUG_ON() since system can continue without network > namespace failed in loopback init path, unless it is > init_net namespace where we panic() anyway. > > Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> Is the BUG_ON() triggering, if so why? It looks to me that unless there is a bug, this assignment is unnecessary.
> From: Serhey Popovych <serhe.popovych@gmail.com> > Date: Fri, 16 Jun 2017 15:10:03 +0300 > >> Now with commit 9c7dafb (net: Allow to create links with >> given ifindex) support registration of network devices >> with specific ifindex is added. >> >> We can force loopback network device index before call to >> register_netdev() to ensure we always configure it with >> LOOPBACK_IFINDEX. >> >> Kill BUG_ON() since system can continue without network >> namespace failed in loopback init path, unless it is >> init_net namespace where we panic() anyway. >> >> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> > > Is the BUG_ON() triggering, if so why? > > It looks to me that unless there is a bug, this assignment > is unnecessary. > Okay, get your position, thanks!
> >> From: Serhey Popovych <serhe.popovych@gmail.com> >> Date: Fri, 16 Jun 2017 15:10:03 +0300 >> >>> Now with commit 9c7dafb (net: Allow to create links with >>> given ifindex) support registration of network devices >>> with specific ifindex is added. >>> >>> We can force loopback network device index before call to >>> register_netdev() to ensure we always configure it with >>> LOOPBACK_IFINDEX. >>> >>> Kill BUG_ON() since system can continue without network >>> namespace failed in loopback init path, unless it is >>> init_net namespace where we panic() anyway. >>> >>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> >> >> Is the BUG_ON() triggering, if so why? >> >> It looks to me that unless there is a bug, this assignment >> is unnecessary. >> > Okay, get your position, thanks! > Sorry for raising again, but my objectives following: BUG_ON() might be compiled out when CONFIG_BUG=n. That's open possibility in buggy setups to have "lo" with ->ifindex other than LOOPBACK_IFINDEX. By forcing register_netdevice() via setting ifindex = LOOPBACK_IFINDEX we address CONFIG_BUG=n case. We can leave BUG_ON(), but add ifindex = LOOPBACK_IFINDEX to ensure register_netdevice() operates properly. What do you think?
From: Serhey Popovych <serhe.popovych@gmail.com> Date: Wed, 21 Jun 2017 12:35:12 +0300 > >> >>> From: Serhey Popovych <serhe.popovych@gmail.com> >>> Date: Fri, 16 Jun 2017 15:10:03 +0300 >>> >>>> Now with commit 9c7dafb (net: Allow to create links with >>>> given ifindex) support registration of network devices >>>> with specific ifindex is added. >>>> >>>> We can force loopback network device index before call to >>>> register_netdev() to ensure we always configure it with >>>> LOOPBACK_IFINDEX. >>>> >>>> Kill BUG_ON() since system can continue without network >>>> namespace failed in loopback init path, unless it is >>>> init_net namespace where we panic() anyway. >>>> >>>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> >>> >>> Is the BUG_ON() triggering, if so why? >>> >>> It looks to me that unless there is a bug, this assignment >>> is unnecessary. >>> >> Okay, get your position, thanks! >> > > Sorry for raising again, but my objectives following: > > BUG_ON() might be compiled out when CONFIG_BUG=n. > That's open possibility in buggy setups to have > "lo" with ->ifindex other than LOOPBACK_IFINDEX. > > By forcing register_netdevice() via setting > ifindex = LOOPBACK_IFINDEX we address CONFIG_BUG=n > case. > > We can leave BUG_ON(), but add ifindex = LOOPBACK_IFINDEX > to ensure register_netdevice() operates properly. > > What do you think? I don't understand what you are trying to achieve. BUG_ON() is an assertion. It checks that an invariant holds. Whether the check is enabled or not has no bearing on the invariant. There is no reason to make the assignment if it is supposed to be set properly already, period. The assertion states that it is supposed to be set properly when we get to this code.
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c index 3061249..d7233aa 100644 --- a/drivers/net/loopback.c +++ b/drivers/net/loopback.c @@ -210,12 +210,13 @@ static __net_init int loopback_net_init(struct net *net) if (!dev) goto out; + dev->ifindex = LOOPBACK_IFINDEX; + dev_net_set(dev, net); err = register_netdev(dev); if (err) goto out_free_netdev; - BUG_ON(dev->ifindex != LOOPBACK_IFINDEX); net->loopback_dev = dev; return 0;
Now with commit 9c7dafb (net: Allow to create links with given ifindex) support registration of network devices with specific ifindex is added. We can force loopback network device index before call to register_netdev() to ensure we always configure it with LOOPBACK_IFINDEX. Kill BUG_ON() since system can continue without network namespace failed in loopback init path, unless it is init_net namespace where we panic() anyway. Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> --- drivers/net/loopback.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)