Re: [PATCH net-next v10 2/4] net: Introduce generic failover module

On Mon, May 07, 2018 at 05:24:27PM -0700, Samudrala, Sridhar wrote:

On 5/7/2018 4:53 PM, Stephen Hemminger wrote:

quoted

On Mon,  7 May 2018 15:10:44 -0700
Sridhar Samudrala [off-list ref] wrote:

quoted

+static struct net_device *net_failover_get_bymac(u8 *mac,
+						 struct net_failover_ops **ops)
+{
+	struct net_device *failover_dev;
+	struct net_failover *failover;
+
+	spin_lock(&net_failover_lock);
+	list_for_each_entry(failover, &net_failover_list, list) {
+		failover_dev = rtnl_dereference(failover->failover_dev);
+		if (ether_addr_equal(failover_dev->perm_addr, mac)) {
+			*ops = rtnl_dereference(failover->ops);
+			spin_unlock(&net_failover_lock);
+			return failover_dev;
+		}
+	}
+	spin_unlock(&net_failover_lock);
+	return NULL;
+}

This is broken if non-ethernet devices such as Infiniband are present.

There is check to make sure that a slave and failover devices are of the same type in
net_failover_slave_register()

	failover_dev = net_failover_get_bymac(slave_dev->perm_addr, &nfo_ops);
        if (!failover_dev)
                goto done;

        if (failover_dev->type != slave_dev->type)
                goto done;

Do you think this is not good enough? I had an explicit check for ARPHRD_ETHER in
earlier patchsets, but removed it based on Jiri's comment.

Right but how is ether_addr_equal supposed to work if types are
identical but not ethernet?

This can also benefit from a comment referring to the check in
net_failover_slave_register.

-- 
MST