Re: [RFC PATCH 10/24] net: rbridge: Add elected dtroot

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

On 9/24/2014 7:52 PM, Ahmed Amamou wrote:

> provide function to get the locally elected distributed tree root from the daemon
> in TRILL RFC multiple tree root can be elected but this is not mendatory
> for this first version we use a unique distributed tree root

> Signed-off-by: Ahmed Amamou <ahmed@gandi.net>
> Signed-off-by: Kamel Haddadou <kamel@gandi.net>
> Signed-off-by: François Cachereul <f.cachereul@alphalink.fr>
> Signed-off-by: William Dauchy <william@gandi.net>
> ---
>   net/bridge/br_private.h          |  1 +
>   net/bridge/rbridge/rbr.c         | 17 +++++++++++++++++
>   net/bridge/rbridge/rbr_netlink.c | 21 +++++++++++++++++++++
>   3 files changed, 39 insertions(+)

[...]
> diff --git a/net/bridge/rbridge/rbr.c b/net/bridge/rbridge/rbr.c
> index 41d47db..8d6d37e 100644
> --- a/net/bridge/rbridge/rbr.c
> +++ b/net/bridge/rbridge/rbr.c
> @@ -76,3 +76,20 @@ void br_trill_set_enabled(struct net_bridge *br, unsigned long val)
>   			br_trill_stop(br);
>   	}
>   }
> +
> +int set_treeroot(struct rbr *rbr, uint16_t treeroot)
> +{
> +	if (unlikely(!VALID_NICK(treeroot))) {
> +		pr_warn_ratelimited
> +		    ("rbr_set_treeroot: given tree root not valid\n");
> +		goto set_tree_root_fail;
> +	}
> +	if (rbr->treeroot != treeroot) {
> +		spin_lock_bh(&rbr->br->lock);
> +		rbr->treeroot = treeroot;
> +		spin_unlock_bh(&rbr->br->lock);

    I think the assignment is already atomic.

> +	}
> +	return 0;

    Empty line wouldn't hurt here...

> + set_tree_root_fail:
> +	return ENOENT;
> +}
> diff --git a/net/bridge/rbridge/rbr_netlink.c b/net/bridge/rbridge/rbr_netlink.c
> index ccba5ae..889e6c8 100644
> --- a/net/bridge/rbridge/rbr_netlink.c
> +++ b/net/bridge/rbridge/rbr_netlink.c
> @@ -56,7 +56,28 @@ static int trill_cmd_add_nicks_info(struct sk_buff *skb, struct genl_info *info)
>   static int trill_cmd_set_treeroot_id(struct sk_buff *skb,
>   				     struct genl_info *info)
>   {
> +	int err = -EINVAL;
> +	u16 nickname;
> +	struct trill_nl_header *trnlhdr;
> +	struct net_device *source_port = NULL;
> +	struct net *net = sock_net(skb->sk);
> +	struct net_bridge_port *p = NULL;

    Need empty line here.

> +	nickname = nla_get_u16(info->attrs[TRILL_ATTR_U16]);
> +	trnlhdr = info->userhdr;
> +	if (trnlhdr->ifindex)
> +		source_port = __dev_get_by_index(net, trnlhdr->ifindex);
> +	if (!source_port)
> +		goto fail;
> +	p = br_port_get_rcu(source_port);
> +	if (!p || !(p->br) || !(p->br->rbr))

    Inner () not needed.

> +		goto fail;
> +	err = set_treeroot(p->br->rbr, htons(nickname));
> +	if (err)
> +		goto fail;
>   	return 0;

    Empty lien wouldn't hurt here...

> + fail:
> +	printk(KERN_WARNING "trill_cmd_set_treeroot_id FAILED\n");

    Use pr_warn() instead.

[...]

WBR, Sergei