netdev - Re: [RFC v4 1/1] net/hsr: Support for the HSR protocol (IEC:2010 / HSR v0)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1350077975.26909.17.camel@joe-AO722>
Date:	Fri, 12 Oct 2012 14:39:35 -0700
From:	Joe Perches <joe@...ches.com>
To:	Arvid Brodin <Arvid.Brodin@...n.com>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"shemminger@...tta.com" <shemminger@...tta.com>,
	"jboticario@...il.com" <jboticario@...il.com>,
	"balferreira@...glemail.com" <balferreira@...glemail.com>
Subject: Re: [RFC v4 1/1] net/hsr: Support for the HSR protocol (IEC:2010 /
 HSR v0)

On Fri, 2012-10-12 at 17:11 +0000, Arvid Brodin wrote:
> This would add support for the High-availability Seamless Redundancy network
> protocol, version 0 (2010).
> 
> This RFC is NOT meant for mainline inclusion at the moment since we're trying
> to figure out how to handle a probable incompatibility with the newer
> HSR v1 (2012) standard.

just trivial comments, ignore as you choose.

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
[]
> +void hsr_set_operstate(struct net_device *hsr_dev, struct net_device *slave1,
> +		       struct net_device *slave2)
> +{
> +	if (!is_admin_up(hsr_dev)) {
> +		__hsr_set_operstate(hsr_dev, IF_OPER_DOWN);
> +		return;
> +	}
> +
> +	if (is_operstate_up(slave1) || is_operstate_up(slave2))
> +		__hsr_set_operstate(hsr_dev, IF_OPER_UP);
> +	else
> +		__hsr_set_operstate(hsr_dev, IF_OPER_LOWERLAYERDOWN);
> +}

sometimes it's better to have a single call like
{
	int type;

	if (!is_admin_up(hsr_dev))
		type = IF_OPER_DOWN;
	else if (is_operstate_up(slave1) || is_operstate_up(slave2))
		type = IF_OPER_UP;
	else
		type = IF_OPER_LOWERLAYERDOWN;

	__hsr_set_operstate(hsr_dev, type);
}

[]

> +static int hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct hsr_priv *hsr_priv;
> +	struct hsr_ethhdr *hsr_ethhdr;
> +	struct sk_buff *skb2;
> +	int res1, res2;
> +
> +	hsr_priv = netdev_priv(dev);
> +	hsr_ethhdr = (struct hsr_ethhdr *) skb->data;
> +
> +	if ((ntohs(skb->protocol) != ETH_P_HSR) ||
> +			(ntohs(hsr_ethhdr->ethhdr.h_proto) != ETH_P_HSR)) {

These ntohs calls are done at runtime.
The conversion could be done at compile time instead using

	if (skb->protocol != ntohs(ETH_P_HSR) ||
	    hsr_ethhdr->ethhdr.h_proto != ntohs(ETH_P_HSR)) {

[]

> +static void restore_slaves(struct net_device *hsr_dev)
> +{
[]
> +			netdev_info(hsr_dev, "HSR: Cannot restore slave "
> +						"promiscuity (%s, %d)\n",
> +						hsr_priv->slave[i]->name, res);

It's nicer to coalesce formats for easier grep

			netdev_info(hsr_dev, "HSR: Cannot restore slave promiscuity (%s, %d)\n",
				    hsr_priv->slave[i]->name, res);

or maybe use special wrappers for something like:
int hsr_info(hsr, slave, fmt, ...)
{
}

[]

> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c

> +static struct node_entry *find_node_by_AddrB(struct list_head *node_db,
> +					     unsigned char addr[ETH_ALEN])
> +{
> +	struct node_entry *node;
> +
> +	list_for_each_entry_rcu(node, node_db, mac_list)
> +		if (!compare_ether_addr(node->MacAddressB, addr))
> +			return node;

please use braces when the list/for has an if


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html