Re: [patch 3/3] bridge: netlink interface for link management

[Stephen Hemminger <shemminger@osdl.org>]

On Wed, 24 May 2006 14:11:55 -0400
jamal <hadi@cyberus.ca> wrote:

> 
> Very nice.
> 
> On Wed, 2006-24-05 at 10:12 -0700, Stephen Hemminger wrote:
> > plain text document attachment (bridge-netlink.patch)
> > Add basic netlink support to the Ethernet bridge. Including:
> >  * dump interfaces in bridges
> >  * monitor link status changes
> >  * change state of bridge port
> 
> 
> > +static int br_fill_ifinfo(struct sk_buff *skb, const struct net_bridge_port *port,
> > +			  u32 pid, u32 seq, int event, unsigned int flags)
> > +{
> > +	const struct net_bridge *br = port->br;
> > +	const struct net_device *dev = port->dev;
> > +	struct ifinfomsg *r;
> > +	struct nlmsghdr *nlh;
> > +	unsigned char *b = skb->tail;
> > +	u32 mtu = dev->mtu;
> > +	u8 operstate = netif_running(dev) ? dev->operstate : IF_OPER_DOWN;
> > +	u8 portstate = port->state;
> > +
> > +	pr_debug("br_fill_info event %d port %s master %s\n",
> > +		 event, dev->name, br->dev->name);
> > +
> > +	nlh = NLMSG_NEW(skb, pid, seq, event, sizeof(*r), flags);
> > +	r = NLMSG_DATA(nlh);
> > +	r->ifi_family = AF_BRIDGE;
> > +	r->__ifi_pad = 0;
> > +	r->ifi_type = dev->type;
> > +	r->ifi_index = dev->ifindex;
> > +	r->ifi_flags = dev_get_flags(dev);
> > +	r->ifi_change = 0;
> > +
> > +	RTA_PUT(skb, IFLA_IFNAME, strlen(dev->name)+1, dev->name);
> > +
> > +	RTA_PUT(skb, IFLA_MASTER, sizeof(int), &br->dev->ifindex);
> > +
> > +	if (dev->addr_len)
> > +		RTA_PUT(skb, IFLA_ADDRESS, dev->addr_len, dev->dev_addr);
> > +
> > +	RTA_PUT(skb, IFLA_MTU, sizeof(mtu), &mtu);
> > +	if (dev->ifindex != dev->iflink)
> > +		RTA_PUT(skb, IFLA_LINK, sizeof(int), &dev->iflink);
> > +
> > +
> > +	RTA_PUT(skb, IFLA_OPERSTATE, sizeof(operstate), &operstate);
> > +
> > +	if (event == RTM_NEWLINK)
> > +		RTA_PUT(skb, IFLA_PROTINFO, sizeof(portstate), &portstate);
> > +
> > +	nlh->nlmsg_len = skb->tail - b;
> > +
> > +	return skb->len;
> > +
> 
> Is it desirable to do the above? link events already happen for each
> netdevice, no? If not it would probably make more sense to export the
> link netlink code (instead of replicating it as above) and just call it
> for each netdevice.

The STP daemon only wants to know about interfaces in bridge, and it needs
to know the bridge port state.

> I think it is worth reporting all the children (their ifindices) of a
> bridge when eventing.

can easily track this in daemon.


> BTW, Where is the bridge state (as in learning, blocking etc) carried? 

In IFLA_PROTINFO

> Having events for that i would deem as valuable.
> 
> > +static int br_rtm_setlink(struct sk_buff *skb,  struct nlmsghdr *nlh, void *arg)
> > +{
> > +	struct rtattr  **rta = arg;
> > +	struct ifinfomsg *ifm = NLMSG_DATA(nlh);
> > +	struct net_device *dev;
> > +	struct net_bridge_port *p;
> > +	u8 new_state;
> > +
> > +	if (ifm->ifi_family != AF_BRIDGE)
> > +		return -EPFNOSUPPORT;
> > +
> > +	/* Must pass valid state as PROTINFO */
> > +	if (rta[IFLA_PROTINFO-1]) {
> 
> Why not a noun like IFLA_BRIDGE_STATE ?

Laziness.  Just used "protocol specific portion"

> 
> 
> > +	/* if kernel STP is running, don't allow changes */
> > +	if (p->br->stp_enabled)
> > +		return -EBUSY;
> 
> Hopefully above to die some day...
> 
> > +
> > +	p->state = new_state;
> > +	br_log_state(p);
> 
> Assuming the above will generate an event which reports things like the
> children of the bridge and the STP state of the bridge etc i.e things in
> struct net_bridge_port mostly.


The br_log_state just does console printk's.