Re: [PATCH 2.6 NET] Device name changing via rtnetlink

[Thomas Graf <tgraf@suug.ch>]

* jamal <1094932793.2344.82.camel@jzny.localdomain> 2004-09-11 15:59
> So my suggestion is making your code the exception.
> i.e something along the lines of:
> 
> addrchg = 1;
> 
> if (mtu/weight/name related)
> 	addrchng = 0;
> if (!err & addrchang)
> 	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);

OK, however I'd prefer to invert the concept. See patch
below. This would send out the following updates:

ififlags - NETDEV_(UP|DOWN) if IFF_UP has changed
IFLA_MAP - none
IFLA_ADDRESS|IFLA_BROADCAST - NETDEV_CHANGEADDR
IFLA_MTU - NETDEV_CHANGEMTU
IFLA_WEIGHT - none
IFLA_IFNAME - NETDEVCHANGENAME

PATCH:
Only send NETDEV_CHANGEADDR notifies for address and broadcast changes.
Notify is also sent out if only one of the 2 changes is successful.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

--- linux-2.6.9-rc1-bk18.orig/net/core/rtnetlink.c	2004-09-11 16:30:30.000000000 +0200
+++ linux-2.6.9-rc1-bk18/net/core/rtnetlink.c	2004-09-11 23:30:05.000000000 +0200
@@ -265,7 +265,7 @@
 	struct ifinfomsg  *ifm = NLMSG_DATA(nlh);
 	struct rtattr    **ida = arg;
 	struct net_device *dev;
-	int err;
+	int err, send_addr_notify = 0;
 
 	dev = dev_get_by_index(ifm->ifi_index);
 	if (!dev)
@@ -312,6 +312,7 @@
 		err = dev->set_mac_address(dev, RTA_DATA(ida[IFLA_ADDRESS - 1]));
 		if (err)
 			goto out;
+		send_addr_notify = 1;
 	}
 
 	if (ida[IFLA_BROADCAST - 1]) {
@@ -319,6 +320,7 @@
 			goto out;
 		memcpy(dev->broadcast, RTA_DATA(ida[IFLA_BROADCAST - 1]),
 		       dev->addr_len);
+		send_addr_notify = 1;
 	}
 
 	if (ida[IFLA_MTU - 1]) {
@@ -365,7 +367,7 @@
 	err = 0;
 
 out:
-	if (!err)
+	if (send_addr_notify)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
 
 	dev_put(dev);


> Also note, the semantics of netlink are all-or-nothing transaction. i.e
> if one of the things requested for fails then you undo the rest. We can
> probably loosely say that unles the ATOMIC flag is set you dont need to
> undo that .. something to think about (summary is you probably dont want
> to send progress update to user space unless the transaction is
> successful; you however want to report progress of where failure happens
> - as we are discussing at the moment in the error code thread).

Agreed, I think undo should happen from user space though. In the
example of do_setlink, you can see that while it would be quite easy to
do the sanity checks before actually doing something you can't catch
all errors if you don't want to implement the name/mtu/addr change
routines yourself. Keeping in mind that those sanity checks will
probably only fail during the development of a application it doesn't
really help. That's why I implemented it this way.

I'm experimenting with a commit/fallback netlink library which basically
fetches the subject of change and stores the netlink message to be used
in case of fallback. NLM_F_ATOMIC makes it even more useable but is
quite expensive. A netlink change daemon would probably solve the
problem better.