Messages in this thread |  | | Date | Wed, 13 Apr 2011 15:32:04 +0400 | From | Vasiliy Kulikov <> | Subject | Re: [PATCH] net: ipv4: add IPPROTO_ICMP socket kind |
| |
On Wed, Apr 13, 2011 at 13:29 +0300, Alexey Dobriyan wrote: > On Sat, Apr 9, 2011 at 1:15 PM, Vasiliy Kulikov <segoon@openwall.com> wrote: > > This patch adds IPPROTO_ICMP socket kind. > > > + seq_printf(f, "%5d: %08X:%04X %08X:%04X" > > + " %02X %08X:%08X %02X:%08lX %08X %5d %8d %lu %d %p %d%n", > > + bucket, src, srcp, dest, destp, sp->sk_state, > > + sk_wmem_alloc_get(sp), > > + sk_rmem_alloc_get(sp), > > + 0, 0L, 0, sock_i_uid(sp), 0, sock_i_ino(sp), > > These zeroes can be embedded into format string for slightly faster printing.
Is it really needed? I mean, it is not a fast path, so such a small overhead is not very bad. But embedding them into the string makes it a bit more difficult to read.
> > +static const struct file_operations ping_seq_fops = { > > + .owner = THIS_MODULE, > > Unnecessary line. > ->owner is unused for proc files, this is not documented anywhere, but > it's unused.
OK.
> > +static const char ping_proc_name[] = "icmp"; > > Ewww :-) > Does not compiler create only one string?
I used it for better readability as it is used 2 times.
> > + p = proc_create_data(ping_proc_name, S_IRUGO, net->proc_net, > > + &ping_seq_fops, NULL); > > There is proc_net_fops_create().
OK.
> > +#ifdef CONFIG_IP_PING > > + table[7].data = > > + &net->ipv4.sysctl_ping_group_range; > > +#endif > > Now I understand it's not related, but next sysctl will have > "table[8].data = ..." line which is off-by-one if CONFIG_IP_PING=n.
Agreed that hardcoded indexes look a bit ugly, especially with configurable elements. But as Dave suggested to completely remove CONFIG_IP_PING, it doesn't make sense now.
Thank you,
-- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |