Re: [patch v2, kernel version 3.2.1] net/ipv4/ip_gre: Ethernetmultipoint GRE over IP

[Štefan Gula]

DÅa 17. januÃra 2012 5:47, Eric Dumazet <eric.dumazet@xxxxxxxxx> napÃsal/a:
> Le mardi 17 janvier 2012 Ã 00:11 +0100, Åtefan Gula a Ãcrit :
>
>> I agree with you, but for the start of this feature I believe static
>> slots size is enough here - same limitation is inside the original
>> linux bridge code. I have merged hopefully all your comments and here
>> is the newest patch:
>
>
>
> 1) Before sending a new version of your patch, please fix your mailer,
> you can read Documentation/email-clients.txt for hints.
>
Sorry, I have to test it as I am using clasical gmail web GUI to send emails

> Send it to you and check you can apply it.
> Then, once you are confident its OK, you can send it to netdev.
>
> Right now, it doesnt apply, because of unexpected line wraps.
>
> # cd next-next
> # cat /tmp/patch | patch -p1
> patching file include/net/ipip.h
> patching file net/ipv4/Kconfig
> patching file net/ipv4/ip_gre.c
> patch: **** malformed patch at line 495: ipgre_tap_bridge_entry *entry)
>
>
> 2) You call ipgre_tap_bridge_fini() from ipgre_exit_net() and
> ipgre_init_net(), thats completely bogus if CONFIG_NET_NS=y
>
> Just remove the struct kmem_cache *ipgre_tap_bridge_cache
> and use instead kmalloc(sizeof(...))/kfree(ptr) instead.
>
As this is completely the same part of code from net/bridge/br_fdb.c,
can you give me a hint about how change that as I believe it should be
changed also there?

> 3) ipgre_tap_bridge_init() should not be __net_init, but __init
>
Ok

>
> 4) I cant find why you store 'struct net_device *dev;' in a
> ipgre_tap_bridge_entry, it seems you never read it ?
>
yes you are right, it is not needed - removed from code

> 5) Also please add an empty line between variable declaration and
> function body. Also, we prefer an alignement of parameters.
>
I used scripts/checkpatch.pl to check my coding styles, but apparently
this is missing from it as it never complains before about this.
Anyway hopefully done ok based your comments

> For example :
>
> static __be32 ipgre_tap_bridge_get_raddr(struct ip_tunnel *tunnel,
> Â Â Â Âconst unsigned char *addr)
> {
> Â Â Â Â__be32 raddr;
> Â Â Â Âstruct ipgre_tap_bridge_entry *entry;
> Â Â Â Ârcu_read_lock();
> Â Â Â Âentry = __ipgre_tap_bridge_get(tunnel, addr);
> Â Â Â Âif (entry == NULL)
> Â Â Â Â Â Â Â Âraddr = 0;
> Â Â Â Âelse
> Â Â Â Â Â Â Â Âraddr = entry->raddr;
> Â Â Â Ârcu_read_unlock();
> Â Â Â Âreturn raddr;
> }
>
> should be :
>
> static __be32 ipgre_tap_bridge_get_raddr(struct ip_tunnel *tunnel,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â const unsigned char *addr)
> {
> Â Â Â Â__be32 raddr = 0;
> Â Â Â Âstruct ipgre_tap_bridge_entry *entry;
>
> Â Â Â Ârcu_read_lock();
> Â Â Â Âentry = __ipgre_tap_bridge_get(tunnel, addr);
> Â Â Â Âif (entry)
> Â Â Â Â Â Â Â Âraddr = entry->raddr;
> Â Â Â Ârcu_read_unlock();
>
> Â Â Â Âreturn raddr;
> }
>
>
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/