lkml.org 
[lkml]   [2011]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2011-04-13 13:35    [W:0.086 / U:11.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site