[PATCH] network device renaming sysfs fix

[PATCH] network device renaming sysfs fix

[Dan Aloni]

(repost, now will hopefully reach the mailing list)

I believe this is a better approach for fixing the sysfs renaming
discrepancy. Later I'll also look into fixing the same issue 
with sysctl.

--- linux/net/core/dev.c	2003-08-15 12:47:50.000000000 +0300
+++ linux/net/core/dev.c	2003-08-15 12:47:59.000000000 +0300
@@ -2347,10 +2347,14 @@
 				return -EEXIST;
 			memcpy(dev->name, ifr->ifr_newname, IFNAMSIZ);
 			dev->name[IFNAMSIZ - 1] = 0;
-			strlcpy(dev->class_dev.class_id, dev->name, BUS_ID_SIZE);
+
+			err = class_device_rename(&dev->class_dev, dev->name);
+			if (err) 
+				printk(KERN_DEBUG "SIOCSIFNAME: error renaming class_device (%d)\n", err);
+
 			notifier_call_chain(&netdev_chain,
 					    NETDEV_CHANGENAME, dev);
-			return 0;
+			return err;
 
 		/*
 		 *	Unknown or private ioctl



-- 
Dan Aloni
da-x@gmx.net

RE: [PATCH] network device renaming sysfs fix

[Hen, Shmulik]

> -----Original Message-----
> From: Dan Aloni [mailto:da-x@gmx.net]
> Sent: Friday, August 15, 2003 2:15 PM
> To: Linux Net-Dev
> Cc: David S. Miller; Mark Huth
> Subject: [PATCH] network device renaming sysfs fix
> 
> 
> (repost, now will hopefully reach the mailing list)
> 
> I believe this is a better approach for fixing the sysfs renaming
> discrepancy. Later I'll also look into fixing the same issue 
> with sysctl.
> 
> 
> -- 
> Dan Aloni
> da-x@gmx.net
> 
> 

In general, it a similar thing applicable for proc fs too ?
We just had to fix something like that in bonding, and
it sounds odd each driver should handle this on it's own.

-- 
| Shmulik Hen   Advanced Network Services  |
| Israel Design Center, Jerusalem          |
| LAN Access Division, Platform Networking |
| Intel Communications Group, Intel corp.  |

Re: [PATCH] network device renaming sysfs fix

[Dan Aloni]

On Fri, Aug 15, 2003 at 02:36:27PM +0300, Hen, Shmulik wrote:
> > -----Original Message-----
> > From: Dan Aloni [mailto:da-x@gmx.net]
> > Sent: Friday, August 15, 2003 2:15 PM
> > To: Linux Net-Dev
> > Cc: David S. Miller; Mark Huth
> > Subject: [PATCH] network device renaming sysfs fix
> > 
> > 
> > (repost, now will hopefully reach the mailing list)
> > 
> > I believe this is a better approach for fixing the sysfs renaming
> > discrepancy. Later I'll also look into fixing the same issue 
> > with sysctl.
> > 
> > 
> > -- 
> > Dan Aloni
> > da-x@gmx.net
> > 
> > 
> 
> In general, it a similar thing applicable for proc fs too ?
> We just had to fix something like that in bonding, and
> it sounds odd each driver should handle this on it's own.

It is more complicated. procfs's interface to sysctl is optional -
thus sysctl's interfaces hide procfs from you. Fixing this require 
changes in sysctl itself, and most probably also in the networking 
code, because network devices' sysctls are not exposed in the 
net/dev/core.c level.

-- 
Dan Aloni
da-x@gmx.net

Re: [PATCH] network device renaming sysfs fix

[Stephen Hemminger]

On Fri, 15 Aug 2003 14:15:14 +0300
Dan Aloni <da-x@gmx.net> wrote:

> (repost, now will hopefully reach the mailing list)
> 
> I believe this is a better approach for fixing the sysfs renaming
> discrepancy. Later I'll also look into fixing the same issue 
> with sysctl.
> 

Yes, this is a better fix because it won't break when unregister and free are split
into seperate steps (coming soon).

But, do we really need the debug message?  
Why return an error if you already committed the change? If you get an error,
then don't rename the network device.

The class_dev kobject name is 20 chars and IFNAMSIZ is 16, so the solution
needs to truncate if new_name is not null terminated (all 16 chars long).

This seems like the best approach:

diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	Fri Aug 15 09:34:11 2003
+++ b/net/core/dev.c	Fri Aug 15 09:34:11 2003
@@ -2343,14 +2343,18 @@
 		case SIOCSIFNAME:
 			if (dev->flags & IFF_UP)
 				return -EBUSY;
+			ifr->ifr_newname[IFNAMSIZ-1] = '\0';
 			if (__dev_get_by_name(ifr->ifr_newname))
 				return -EEXIST;
-			memcpy(dev->name, ifr->ifr_newname, IFNAMSIZ);
-			dev->name[IFNAMSIZ - 1] = 0;
-			strlcpy(dev->class_dev.class_id, dev->name, BUS_ID_SIZE);
-			notifier_call_chain(&netdev_chain,
-					    NETDEV_CHANGENAME, dev);
-			return 0;
+			err = class_device_rename(&dev->class_dev, 
+						  ifr->ifr_newname);
+			if (!err) {
+				strlcpy(dev->name, ifr->ifr_newname, IFNAMSIZ);
+
+				notifier_call_chain(&netdev_chain,
+						    NETDEV_CHANGENAME, dev);
+			}
+			return err;
 
 		/*
 		 *	Unknown or private ioctl

Re: [PATCH] network device renaming sysfs fix

[Dan Aloni]

On Fri, Aug 15, 2003 at 09:37:24AM -0700, Stephen Hemminger wrote:
> Yes, this is a better fix because it won't break when unregister and free are split
> into seperate steps (coming soon).
> 
> But, do we really need the debug message?  
> Why return an error if you already committed the change? If you get an error,
> then don't rename the network device.
> 
> The class_dev kobject name is 20 chars and IFNAMSIZ is 16, so the solution
> needs to truncate if new_name is not null terminated (all 16 chars long).
> 
> This seems like the best approach:

Agreed.

> diff -Nru a/net/core/dev.c b/net/core/dev.c
> --- a/net/core/dev.c	Fri Aug 15 09:34:11 2003
> +++ b/net/core/dev.c	Fri Aug 15 09:34:11 2003
[patch]
-- 
Dan Aloni
da-x@gmx.net

Re: [PATCH] network device renaming sysfs fix

[Mark Huth]

Agreed.  We had not yet found the class_device_rename.
Hopefully, this is no longer in HTML format.
By all means, help not to look like a bozo :-)  - a tall order, I realize.

Mark Huth

Dan Aloni wrote:

>(repost, now will hopefully reach the mailing list)
>
>I believe this is a better approach for fixing the sysfs renaming
>discrepancy. Later I'll also look into fixing the same issue 
>with sysctl.
>
>--- linux/net/core/dev.c	2003-08-15 12:47:50.000000000 +0300
>+++ linux/net/core/dev.c	2003-08-15 12:47:59.000000000 +0300
>@@ -2347,10 +2347,14 @@
> 				return -EEXIST;
> 			memcpy(dev->name, ifr->ifr_newname, IFNAMSIZ);
> 			dev->name[IFNAMSIZ - 1] = 0;
>-			strlcpy(dev->class_dev.class_id, dev->name, BUS_ID_SIZE);
>+
>+			err = class_device_rename(&dev->class_dev, dev->name);
>+			if (err) 
>+				printk(KERN_DEBUG "SIOCSIFNAME: error renaming class_device (%d)\n", err);
>+
> 			notifier_call_chain(&netdev_chain,
> 					    NETDEV_CHANGENAME, dev);
>-			return 0;
>+			return err;
> 
> 		/*
> 		 *	Unknown or private ioctl
>
>
>
>  
>

Re: [PATCH] network device renaming sysfs fix

[David S. Miller]

On Fri, 15 Aug 2003 09:37:24 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> The class_dev kobject name is 20 chars and IFNAMSIZ is 16, so the solution
> needs to truncate if new_name is not null terminated (all 16 chars long).
> 
> This seems like the best approach:

Patch applied, thanks everyone.

Can't the device name also change via netlink messages?
If so, do we handle that case properly?

Re: [PATCH] network device renaming sysfs fix

[Dan Aloni]

On Sat, Aug 16, 2003 at 07:40:14AM -0700, David S. Miller wrote:
> On Fri, 15 Aug 2003 09:37:24 -0700
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > The class_dev kobject name is 20 chars and IFNAMSIZ is 16, so the solution
> > needs to truncate if new_name is not null terminated (all 16 chars long).
> > 
> > This seems like the best approach:
> 
> Patch applied, thanks everyone.
> 
> Can't the device name also change via netlink messages?
> If so, do we handle that case properly?

No it can't, AFAIK. You might be confusing between netlink and 
notifier calls (which allow ipv4 to update the names of aliases
when the device name changes).

BTW, it is possible to add notifier handling for fixing device 
naming discrepancies regarding family-level sysctl registration. 
I think this is how the sysctl issue should be fixed.

-- 
Dan Aloni
da-x@gmx.net