On Tuesday 24 November 2009, Patrick McHardy wrote:

Arnd Bergmann wrote:

quoted

@@ -29,9 +29,16 @@
 #include <linux/if_link.h>
 #include <linux/if_macvlan.h>
 #include <net/rtnetlink.h>
+#include <net/xfrm.h>

Do we really need this?

Not any more. I originally did secpath_reset here, but that is now moved
to dev_forward_skb() in net/core/dev.c so I'll remove the include here.

quoted

@@ -129,11 +137,14 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
 }
 
 static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
-				 const struct ethhdr *eth)
+				 const struct ethhdr *eth, int local)

bool local?

Already changed as a reply to Eric's comments

quoted

@@ -159,8 +172,12 @@ static void macvlan_broadcast(struct sk_buff *skb,
 
 	for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
 		hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
+			if ((vlan->dev == src) || !(vlan->mode & mode))

Please remove those unnecessary parentheses around the
device comparison.

Ok. I usually prefer to have some extra parentheses in places that not
everyone finds obvious but these are pretty clear.

The '|' should go on the first line.

Multi-line comments should begin every line with '*'.

Please align the mode values (in all cases above) to the arguments
on the line above.

ok

quoted

+static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	const struct macvlan_dev *vlan = netdev_priv(dev);
+	const struct macvlan_port *port = vlan->port;
+	const struct macvlan_dev *dest;
+
+	if (vlan->mode == MACVLAN_MODE_BRIDGE) {
+		const struct ethhdr *eth = (void *)skb->data;

eth_hdr()?

This is done before calling eth_type_trans and skb->mac_header
appears to be uninitialized then, which gave me an instant kernel
panic here.
 

quoted

+		dest = macvlan_hash_lookup(port, eth->h_dest);
+		if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
+			int length = skb->len + ETH_HLEN;

unsigned int for length values please.

ok

Thanks for the detailed review,

	Arnd <><