| Message ID | 1405606186-13703-2-git-send-email-vfalico@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: > netdev_name() returns dev->name only when the net_device is in > NETREG_REGISTERED state. > > However, dev->name is always populated on creation, so we can easily use > it. > > There are two cases when there's no real name - when it's an empty string > or when the name is in form of "eth%d", then netdev_name() returns "unnamed > net_device". > > CC: "David S. Miller" <davem@davemloft.net> > CC: Tom Gundersen <teg@jklm.no> > Signed-off-by: Veaceslav Falico <vfalico@gmail.com> > --- > > Notes: > v1->v2: > Also account for an empty string, as Tom Gundersen suggested. > > include/linux/netdevice.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 15ed750..70256aa 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -3383,8 +3383,8 @@ extern struct pernet_operations __net_initdata loopback_net_ops; > > static inline const char *netdev_name(const struct net_device *dev) > { > - if (dev->reg_state != NETREG_REGISTERED) > - return "(unregistered net_device)"; > + if (!dev->name[0] || strchr(dev->name, '%')) > + return "(unnamed net_device)"; > return dev->name; > } > Maybe this should not be inline and become something like: const char *netdev_name(const struct net_device *dev) { if (dev->reg_state == NETREG_REGISTERED) return dev->name; if (!dev->name[0]) return "(unnamed net_device)"; if (!strchr(dev->name, '%')) return "(unregistered net_device)"; return dev->name; } EXPORT_SYMBOL(netdev_name); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 09:18:44AM -0700, Joe Perches wrote: >On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: >> netdev_name() returns dev->name only when the net_device is in >> NETREG_REGISTERED state. >> >> However, dev->name is always populated on creation, so we can easily use >> it. >> >> There are two cases when there's no real name - when it's an empty string >> or when the name is in form of "eth%d", then netdev_name() returns "unnamed >> net_device". >> >> CC: "David S. Miller" <davem@davemloft.net> >> CC: Tom Gundersen <teg@jklm.no> >> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> >> --- >> >> Notes: >> v1->v2: >> Also account for an empty string, as Tom Gundersen suggested. >> >> include/linux/netdevice.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index 15ed750..70256aa 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -3383,8 +3383,8 @@ extern struct pernet_operations __net_initdata loopback_net_ops; >> >> static inline const char *netdev_name(const struct net_device *dev) >> { >> - if (dev->reg_state != NETREG_REGISTERED) >> - return "(unregistered net_device)"; >> + if (!dev->name[0] || strchr(dev->name, '%')) >> + return "(unnamed net_device)"; >> return dev->name; >> } >> > >Maybe this should not be inline and become something like: It will miss the states then, when it's not NETREG_REGISTERED. > >const char *netdev_name(const struct net_device *dev) >{ > if (dev->reg_state == NETREG_REGISTERED) > return dev->name; > > if (!dev->name[0]) > return "(unnamed net_device)"; > > if (!strchr(dev->name, '%')) > return "(unregistered net_device)"; > > return dev->name; >} >EXPORT_SYMBOL(netdev_name); > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-07-17 at 18:25 +0200, Veaceslav Falico wrote: > On Thu, Jul 17, 2014 at 09:18:44AM -0700, Joe Perches wrote: > >On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: > >> netdev_name() returns dev->name only when the net_device is in > >> NETREG_REGISTERED state. [] > >Maybe this should not be inline and become something like: > > It will miss the states then, when it's not NETREG_REGISTERED. If it's registered, it has a valid name via dev_get_valid_name() doesn't it? > >const char *netdev_name(const struct net_device *dev) > >{ > > if (dev->reg_state == NETREG_REGISTERED) > > return dev->name; > > > > if (!dev->name[0]) > > return "(unnamed net_device)"; > > > > if (!strchr(dev->name, '%')) > > return "(unregistered net_device)"; > > > > return dev->name; > >} > >EXPORT_SYMBOL(netdev_name); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 09:36:08AM -0700, Joe Perches wrote: >On Thu, 2014-07-17 at 18:25 +0200, Veaceslav Falico wrote: >> On Thu, Jul 17, 2014 at 09:18:44AM -0700, Joe Perches wrote: >> >On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: >> >> netdev_name() returns dev->name only when the net_device is in >> >> NETREG_REGISTERED state. >[] >> >Maybe this should not be inline and become something like: >> >> It will miss the states then, when it's not NETREG_REGISTERED. > >If it's registered, it has a valid name via >dev_get_valid_name() doesn't it? Yes, I'm speaking about the NETREG_* states when it's not registered. i.e.: Jul 17 13:35:29 darkmag kernel: [ 602.686489] bond2 (unregistering): Released all slaves that's with my patchset, when the bond device is unregistering (NETREG_UNREGISTERING). With your patch it would be only "bond2: Released all slaves". As the most time the device is in the NETREG_REGISTERED state, there will be no differencies in the output (with either my or your approach), however in less-common states/codepaths it will also output its state, which might be quite valuable when analyzing the logs. > >> >const char *netdev_name(const struct net_device *dev) >> >{ >> > if (dev->reg_state == NETREG_REGISTERED) >> > return dev->name; >> > >> > if (!dev->name[0]) >> > return "(unnamed net_device)"; >> > >> > if (!strchr(dev->name, '%')) >> > return "(unregistered net_device)"; >> > >> > return dev->name; >> >} >> >EXPORT_SYMBOL(netdev_name); > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 17, 2014 at 09:58:16AM -0700, Joe Perches wrote: >On Thu, 2014-07-17 at 18:38 +0200, Veaceslav Falico wrote: >> On Thu, Jul 17, 2014 at 09:36:08AM -0700, Joe Perches wrote: >> >On Thu, 2014-07-17 at 18:25 +0200, Veaceslav Falico wrote: >> >> On Thu, Jul 17, 2014 at 09:18:44AM -0700, Joe Perches wrote: >> >> >On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: >> >> >> netdev_name() returns dev->name only when the net_device is in >> >> >> NETREG_REGISTERED state. >> >[] >> >> >Maybe this should not be inline and become something like: >> >> >> >> It will miss the states then, when it's not NETREG_REGISTERED. >> > >> >If it's registered, it has a valid name via >> >dev_get_valid_name() doesn't it? >> >> Yes, I'm speaking about the NETREG_* states when it's not registered. >> >> i.e.: >> >> Jul 17 13:35:29 darkmag kernel: [ 602.686489] bond2 (unregistering): Released all slaves > >OK, I hadn't read the patch where netdev_reg_state was emitted. > >Still, it's probably smaller overall code to uninline it now. I don't really care about that, and it's used only in several non-inlined functions anyway, so the difference would be minimal, if any. I can easily re-spin it unlined, however I don't really see a point in doing so :). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-07-17 at 18:38 +0200, Veaceslav Falico wrote: > On Thu, Jul 17, 2014 at 09:36:08AM -0700, Joe Perches wrote: > >On Thu, 2014-07-17 at 18:25 +0200, Veaceslav Falico wrote: > >> On Thu, Jul 17, 2014 at 09:18:44AM -0700, Joe Perches wrote: > >> >On Thu, 2014-07-17 at 16:09 +0200, Veaceslav Falico wrote: > >> >> netdev_name() returns dev->name only when the net_device is in > >> >> NETREG_REGISTERED state. > >[] > >> >Maybe this should not be inline and become something like: > >> > >> It will miss the states then, when it's not NETREG_REGISTERED. > > > >If it's registered, it has a valid name via > >dev_get_valid_name() doesn't it? > > Yes, I'm speaking about the NETREG_* states when it's not registered. > > i.e.: > > Jul 17 13:35:29 darkmag kernel: [ 602.686489] bond2 (unregistering): Released all slaves OK, I hadn't read the patch where netdev_reg_state was emitted. Still, it's probably smaller overall code to uninline it now. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 15ed750..70256aa 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3383,8 +3383,8 @@ extern struct pernet_operations __net_initdata loopback_net_ops; static inline const char *netdev_name(const struct net_device *dev) { - if (dev->reg_state != NETREG_REGISTERED) - return "(unregistered net_device)"; + if (!dev->name[0] || strchr(dev->name, '%')) + return "(unnamed net_device)"; return dev->name; }
netdev_name() returns dev->name only when the net_device is in NETREG_REGISTERED state. However, dev->name is always populated on creation, so we can easily use it. There are two cases when there's no real name - when it's an empty string or when the name is in form of "eth%d", then netdev_name() returns "unnamed net_device". CC: "David S. Miller" <davem@davemloft.net> CC: Tom Gundersen <teg@jklm.no> Signed-off-by: Veaceslav Falico <vfalico@gmail.com> --- Notes: v1->v2: Also account for an empty string, as Tom Gundersen suggested. include/linux/netdevice.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)