[PATCH net-next 0/2] net: print net_device name/state more often

[PATCH net-next 0/2] net: print net_device name/state more often

[Veaceslav Falico]

Hi,

Currently we use net_device->name only if it's the NETREG_REGISTERED
reg_state, otherwise we return "(unregistered device)".

However, we always populate net_device->name on creation, so it's always
available to us for use. The only caveat is that we might have a name like
"eth%d", in which case we cannot use it as it might change in the future.

Also, the net_device might not be NETREG_UNREGISTERED when the function is
called (_UNINITIALIZED, _UNREGISTERING, _RELEASED, _DUMMY), so it's
misleading.

So, a better way would be to always return the dev->name in netdev_name(),
unless it's in the form of "eth%d", then return "unnamed net_device". This
way we'll always return the name in NETREG_REGISTERED reg_state, and also
return it in other states, when possible.

Also, to be more verbose on non-NETREG_REGISTERED states, add a function
netdev_reg_state(), which returns a string describing the state, and use it
in netdev_printk()-related functions. If the dev is in NETREG_REGISTERED
state then a void string is regurned and, thus, nothing changes.

After these two patches we'll have the same behaviour in the usual cases,
and more verbose in non-standardad/buggy ones.

CC: "David S. Miller" <davem@davemloft.net>
CC: Jason Baron <jbaron@akamai.com>
CC: Eric Dumazet <edumazet@google.com>
CC: Vlad Yasevich <vyasevic@redhat.com>
CC: stephen hemminger <stephen@networkplumber.org>
CC: Jerry Chu <hkchu@google.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>

---
 include/linux/netdevice.h | 21 ++++++++++++++++++---
 lib/dynamic_debug.c       |  8 +++++---
 net/core/dev.c            |  8 +++++---
 3 files changed, 28 insertions(+), 9 deletions(-)

[PATCH net-next 1/2] net: use dev->name in netdev_pr* when it's available

[Veaceslav Falico]

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. The only caveat is that the device still has a name of type "eth%d", in
which case we cannot use this name as it'll change in the nearest future,
thus return "unnamed net_device".

CC: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>
---
 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..cde12a3 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 (strchr(dev->name, '%'))
+		return "(unnamed net_device)";
 	return dev->name;
 }
 
-- 
1.8.4

Re: [PATCH net-next 1/2] net: use dev->name in netdev_pr* when it's available

[Tom Gundersen]

On Thu, Jul 17, 2014 at 1:28 PM, Veaceslav Falico <vfalico@gmail.com> wrote:
> e() 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. The only caveat is that the device still has a name of type "eth%d", in
> which case we cannot use this name as it'll change in the nearest future,
> thus return "unnamed net_device".

There are also cases when the name is set to emptystring before being
populated with the real name later on. You may want to check for that
too.

Cheers,

Tom

Re: [PATCH net-next 1/2] net: use dev->name in netdev_pr* when it's available

[Veaceslav Falico]

On Thu, Jul 17, 2014 at 03:48:45PM +0200, Tom Gundersen wrote:
>On Thu, Jul 17, 2014 at 1:28 PM, Veaceslav Falico <vfalico@gmail.com> wrote:
>> e() 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. The only caveat is that the device still has a name of type "eth%d", in
>> which case we cannot use this name as it'll change in the nearest future,
>> thus return "unnamed net_device".
>
>There are also cases when the name is set to emptystring before being
>populated with the real name later on. You may want to check for that
>too.

Sure, it'd be easy enough.

Out of curiosity - can you point me to where it's an empty string?

I'll send v2 now.

Thank you!

>
>Cheers,
>
>Tom

Re: [PATCH net-next 1/2] net: use dev->name in netdev_pr* when it's available

[Tom Gundersen]

On Thu, Jul 17, 2014 at 4:01 PM, Veaceslav Falico <vfalico@gmail.com> wrote:
> On Thu, Jul 17, 2014 at 03:48:45PM +0200, Tom Gundersen wrote:
>>
>> On Thu, Jul 17, 2014 at 1:28 PM, Veaceslav Falico <vfalico@gmail.com>
>> wrote:
>>>
>>> e() 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. The only caveat is that the device still has a name of type "eth%d",
>>> in
>>> which case we cannot use this name as it'll change in the nearest future,
>>> thus return "unnamed net_device".
>>
>>
>> There are also cases when the name is set to emptystring before being
>> populated with the real name later on. You may want to check for that
>> too.
>
>
> Sure, it'd be easy enough.
>
> Out of curiosity - can you point me to where it's an empty string?

Only a handfull places (and I didn't check if those actually use the
print functions before setting the real name):

$ git grep alloc_netdev | grep \"\"
drivers/net/hamradio/dmascc.c:    info->dev[0] = alloc_netdev(0, "",
NET_NAME_UNKNOWN, dev_setup);
drivers/net/hamradio/dmascc.c:    info->dev[1] = alloc_netdev(0, "",
NET_NAME_UNKNOWN, dev_setup);
drivers/net/ppp/ppp_generic.c:    dev = alloc_netdev(sizeof(struct
ppp), "", NET_NAME_UNKNOWN,
drivers/net/wireless/airo.c:    dev = alloc_netdev(sizeof(*ai), "",
NET_NAME_UNKNOWN, ether_setup);
net/atm/clip.c:    dev = alloc_netdev(sizeof(struct clip_priv), "",
NET_NAME_UNKNOWN,

Cheers,

Tom

Re: [PATCH net-next 1/2] net: use dev->name in netdev_pr* when it's available

[Veaceslav Falico]

On Thu, Jul 17, 2014 at 04:29:21PM +0200, Tom Gundersen wrote:
>On Thu, Jul 17, 2014 at 4:01 PM, Veaceslav Falico <vfalico@gmail.com> wrote:
>> On Thu, Jul 17, 2014 at 03:48:45PM +0200, Tom Gundersen wrote:
>>>
>>> On Thu, Jul 17, 2014 at 1:28 PM, Veaceslav Falico <vfalico@gmail.com>
>>> wrote:
>>>>
>>>> e() 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. The only caveat is that the device still has a name of type "eth%d",
>>>> in
>>>> which case we cannot use this name as it'll change in the nearest future,
>>>> thus return "unnamed net_device".
>>>
>>>
>>> There are also cases when the name is set to emptystring before being
>>> populated with the real name later on. You may want to check for that
>>> too.
>>
>>
>> Sure, it'd be easy enough.
>>
>> Out of curiosity - can you point me to where it's an empty string?
>
>Only a handfull places (and I didn't check if those actually use the
>print functions before setting the real name):

Yeah, indeed, missed those.

Sent v2.

Thanks a lot!

>
>$ git grep alloc_netdev | grep \"\"
>drivers/net/hamradio/dmascc.c:    info->dev[0] = alloc_netdev(0, "",
>NET_NAME_UNKNOWN, dev_setup);
>drivers/net/hamradio/dmascc.c:    info->dev[1] = alloc_netdev(0, "",
>NET_NAME_UNKNOWN, dev_setup);
>drivers/net/ppp/ppp_generic.c:    dev = alloc_netdev(sizeof(struct
>ppp), "", NET_NAME_UNKNOWN,
>drivers/net/wireless/airo.c:    dev = alloc_netdev(sizeof(*ai), "",
>NET_NAME_UNKNOWN, ether_setup);
>net/atm/clip.c:    dev = alloc_netdev(sizeof(struct clip_priv), "",
>NET_NAME_UNKNOWN,
>
>Cheers,
>
>Tom

[PATCH net-next 2/2] net: print net_device reg_state in netdev_* unless it's registered

[Veaceslav Falico]

This way we'll always know in what status the device is, unless it's
running normally (i.e. NETDEV_REGISTERED).

CC: "David S. Miller" <davem@davemloft.net>
CC: Jason Baron <jbaron@akamai.com>
CC: Eric Dumazet <edumazet@google.com>
CC: Vlad Yasevich <vyasevic@redhat.com>
CC: stephen hemminger <stephen@networkplumber.org>
CC: Jerry Chu <hkchu@google.com>
CC: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Veaceslav Falico <vfalico@gmail.com>
---
 include/linux/netdevice.h | 17 ++++++++++++++++-
 lib/dynamic_debug.c       |  8 +++++---
 net/core/dev.c            |  8 +++++---
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cde12a3..4e93f14 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3388,6 +3388,20 @@ static inline const char *netdev_name(const struct net_device *dev)
 	return dev->name;
 }
 
+static inline const char *netdev_reg_state(const struct net_device *dev)
+{
+	switch (dev->reg_state) {
+	case NETREG_UNINITIALIZED: return " (unregistered)";
+	case NETREG_REGISTERED: return "";
+	case NETREG_UNREGISTERING: return " (unregistering)";
+	case NETREG_UNREGISTERED: return " (unregistered)";
+	case NETREG_RELEASED: return " (released)";
+	case NETREG_DUMMY: return " (dummy)";
+	}
+
+	return " (unknown)";
+}
+
 __printf(3, 4)
 int netdev_printk(const char *level, const struct net_device *dev,
 		  const char *format, ...);
@@ -3444,7 +3458,8 @@ do {								\
  * file/line information and a backtrace.
  */
 #define netdev_WARN(dev, format, args...)			\
-	WARN(1, "netdevice: %s\n" format, netdev_name(dev), ##args)
+	WARN(1, "netdevice: %s%s\n" format, netdev_name(dev),	\
+	     netdev_reg_state(dev), ##args)
 
 /* netif printk helpers, similar to netdev_printk */
 
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 7288e38..c9afbe2 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -614,13 +614,15 @@ int __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		char buf[PREFIX_SIZE];
 
 		res = dev_printk_emit(7, dev->dev.parent,
-				      "%s%s %s %s: %pV",
+				      "%s%s %s %s%s: %pV",
 				      dynamic_emit_prefix(descriptor, buf),
 				      dev_driver_string(dev->dev.parent),
 				      dev_name(dev->dev.parent),
-				      netdev_name(dev), &vaf);
+				      netdev_name(dev), netdev_reg_state(dev),
+				      &vaf);
 	} else if (dev) {
-		res = printk(KERN_DEBUG "%s: %pV", netdev_name(dev), &vaf);
+		res = printk(KERN_DEBUG "%s%s: %pV", netdev_name(dev),
+			     netdev_reg_state(dev), &vaf);
 	} else {
 		res = printk(KERN_DEBUG "(NULL net_device): %pV", &vaf);
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index 239722a..81d6101 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6950,12 +6950,14 @@ static int __netdev_printk(const char *level, const struct net_device *dev,
 	if (dev && dev->dev.parent) {
 		r = dev_printk_emit(level[1] - '0',
 				    dev->dev.parent,
-				    "%s %s %s: %pV",
+				    "%s %s %s%s: %pV",
 				    dev_driver_string(dev->dev.parent),
 				    dev_name(dev->dev.parent),
-				    netdev_name(dev), vaf);
+				    netdev_name(dev), netdev_reg_state(dev),
+				    vaf);
 	} else if (dev) {
-		r = printk("%s%s: %pV", level, netdev_name(dev), vaf);
+		r = printk("%s%s%s: %pV", level, netdev_name(dev),
+			   netdev_reg_state(dev), vaf);
 	} else {
 		r = printk("%s(NULL net_device): %pV", level, vaf);
 	}
-- 
1.8.4