Received: with ECARTIS (v1.0.0; list netdev); Wed, 01 Dec 2004 23:34:55 -0800 (PST) Received: from mail.osdl.org (fw.osdl.org [65.172.181.6]) by oss.sgi.com (8.13.0/8.13.0) with ESMTP id iB27YpGI024362 for ; Wed, 1 Dec 2004 23:34:51 -0800 Received: from bix (build.pdx.osdl.net [172.20.1.2]) by mail.osdl.org (8.11.6/8.11.6) with SMTP id iB27YM912409; Wed, 1 Dec 2004 23:34:22 -0800 Date: Wed, 1 Dec 2004 23:34:00 -0800 From: Andrew Morton To: Shaun Jackman Cc: netdev@oss.sgi.com Subject: Re: Multicast filtering for tun.c [PATCH] Message-Id: <20041201233400.45078efe.akpm@osdl.org> In-Reply-To: <7f45d939041201140329d0273f@mail.gmail.com> References: <7f45d9390411251715138b35d0@mail.gmail.com> <20041127011006.03232bb6.akpm@osdl.org> <7f45d939041201140329d0273f@mail.gmail.com> X-Mailer: Sylpheed version 0.9.7 (GTK+ 1.2.10; i386-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-archive-position: 12381 X-ecartis-version: Ecartis v1.0.0 Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com X-original-sender: akpm@osdl.org Precedence: bulk X-list: netdev Shaun Jackman wrote: > > This patch adds multicast filtering to the TUN network driver, for > packets being sent from the network device to the character device. It > applies against the 2.6.8.1 kernel tree. > > ... Minor points: > diff -ur linux-2.6.8.1.orig/drivers/net/tun.c linux-2.6.8.1/drivers/net/tun.c > --- linux-2.6.8.1.orig/drivers/net/tun.c 2004-08-14 03:55:23.000000000 -0700 > +++ linux-2.6.8.1/drivers/net/tun.c 2004-11-25 17:00:22.000000000 -0800 > @@ -41,6 +41,7 @@ > #include > #include > #include > +#include You're sure this shouldn't be using crc-ccitt? > +del_multi(u32* filter, const u8* addr) del_multi(u32 *filter, const u8 *addr) would be a more typical layout. > static struct net_device_stats *tun_net_stats(struct net_device *dev) > @@ -301,6 +333,10 @@ > > add_wait_queue(&tun->read_wait, &wait); > while (len) { > + const u8 ones[ ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; The space after the `[' is gratuitous. This array will be allocated onthe stack and populated by hand each time this function is called. It should be made static. > + u8 addr[ ETH_ALEN]; extraneous space. > + memcpy(addr, skb->data, min(sizeof addr, skb->len)); We normally put parentheses around the argument of the sizeof operator, for no particular reason ;) > + if (copy_from_user(&ifr, argp, sizeof ifr)) Ditto > + case SIOCGIFFLAGS: > + ifr.ifr_flags = tun->if_flags; > + if (copy_to_user( argp, &ifr, sizeof ifr)) extraneous space. > + if (copy_to_user( argp, &ifr, sizeof ifr)) ditto > + DBG(KERN_DEBUG "%s: add multi: %x:%x:%x:%x:%x:%x\n", > + tun->dev->name, > + (u8)ifr.ifr_hwaddr.sa_data[0], (u8)ifr.ifr_hwaddr.sa_data[1], > + (u8)ifr.ifr_hwaddr.sa_data[2], (u8)ifr.ifr_hwaddr.sa_data[3], > + (u8)ifr.ifr_hwaddr.sa_data[4], (u8)ifr.ifr_hwaddr.sa_data[5]); Why all the typecasts? > + case SIOCDELMULTI: > + /** Remove the specified group from the character device's multicast > + * filter list. */ > + del_multi(tun->chr_filter, ifr.ifr_hwaddr.sa_data); > + DBG(KERN_DEBUG "%s: del multi: %x:%x:%x:%x:%x:%x\n", > + tun->dev->name, > + (u8)ifr.ifr_hwaddr.sa_data[0], (u8)ifr.ifr_hwaddr.sa_data[1], > + (u8)ifr.ifr_hwaddr.sa_data[2], (u8)ifr.ifr_hwaddr.sa_data[3], > + (u8)ifr.ifr_hwaddr.sa_data[4], (u8)ifr.ifr_hwaddr.sa_data[5]); ditto.