| Message ID | 4AEAAFC4.9050309@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet wrote, On 10/30/2009 10:20 AM: > While preparing a patch for net-next-2.6, I noticed following code in dev_change_name() > > int err = 0; > ... > ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev); > ret = notifier_to_errno(ret); > > if (ret) { > << HERE >> if (err) { > printk(KERN_ERR > "%s: name change rollback failed: %d.\n", > dev->name, ret); > } else { > err = ret; > memcpy(dev->name, oldname, IFNAMSIZ); > goto rollback; > } > } > > > It seems intent was to test if notifier_to_errno() was null ? I don't think so: err stores the previous ret meaning rollback and is checked for this later. But somebody forgot err can store previous (positive) value here, so IMHO you're right: there is a bug in this place ;-) Jarek P. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > > diff --git a/net/core/dev.c b/net/core/dev.c > index b8f74cf..029cd41 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -939,9 +939,9 @@ rollback: > write_unlock_bh(&dev_base_lock); > > ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev); > - ret = notifier_to_errno(ret); > > if (ret) { > + err = notifier_to_errno(ret); > if (err) { > printk(KERN_ERR > "%s: name change rollback failed: %d.\n", > -- > 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 > -- 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
From: Jarek Poplawski <jarkao2@gmail.com> Date: Sat, 31 Oct 2009 00:50:09 +0100 > I don't think so: err stores the previous ret meaning rollback and > is checked for this later. But somebody forgot err can store previous > (positive) value here, so IMHO you're right: there is a bug in this > place ;-) Just not the one Eric is specifically fixing :-) -- 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/net/core/dev.c b/net/core/dev.c index b8f74cf..029cd41 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -939,9 +939,9 @@ rollback: write_unlock_bh(&dev_base_lock); ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev); - ret = notifier_to_errno(ret); if (ret) { + err = notifier_to_errno(ret); if (err) { printk(KERN_ERR "%s: name change rollback failed: %d.\n",
While preparing a patch for net-next-2.6, I noticed following code in dev_change_name() int err = 0; ... ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev); ret = notifier_to_errno(ret); if (ret) { << HERE >> if (err) { printk(KERN_ERR "%s: name change rollback failed: %d.\n", dev->name, ret); } else { err = ret; memcpy(dev->name, oldname, IFNAMSIZ); goto rollback; } } It seems intent was to test if notifier_to_errno() was null ? Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- -- 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