From: Ben Hutchings <bhutchings@solarflare•com>
To: James Cammarata <jimi@sngx•net>
Cc: Eric Dumazet <dada1@cosmosbay•com>,
linux-kernel@vger•kernel.org,
Linux Netdev List <netdev@vger•kernel.org>
Subject: Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs
Date: Sun, 18 May 2008 01:31:05 +0100 [thread overview]
Message-ID: <20080518003104.GK28241@solarflare.com> (raw)
In-Reply-To: <482F610D.2080108@sngx.net>
James Cammarata wrote:
> >This version has a bug, please check again.
>
> Gah, you're right, fixed below.
>
> ---
>
> --- linux-2.6.25.4/net/core/dev.c 2008-05-15 10:00:12.000000000 -0500
> +++ linux-2.6.25.4-jcammara/net/core/dev.c 2008-05-17
> 17:45:57.000000000 -0500
> @@ -2455,2 +2455,80 @@
>
> +/**
> + * proc_net_dev_write - handle writes to /proc/net/dev
> + * @file: not used
> + * @buf: buffer to write
> + * @length: length of buf, at most PAGE_SIZE
> + * @ppos: not used
> + *
> + * Description: this provides a mechanism to clear statistics on a
> + * per-interface basis
> + * "echo 'net clear-stats ifdev' >/proc/net/dev"
> + * with "ifdev" replaced by the device name you wish to clear.
> + *
> + */
> +static ssize_t proc_net_dev_write(struct file *file, const char __user
> *buf,
You need to stop your mail client word-wrapping patches. It looks like
something has converted tabs to spaces, too.
[...]
> + /*
> + * Usage: echo "net clear-stats ifdev" >/proc/net/dev
> + * with "ifdev" replaced by the device name you wish to clear.
> + */
This is redundant with the kernel-doc comment.
> + if (!strncmp("net clear-stats",buffer,15)) {
This doesn't check for a space after, which you rely on later on. (What if
length == 15?) Also, explicitly writing the length of a literal string is
error-prone. Seems like it would be better to do something like:
static const char command[] = "net clear-stats ";
...
if (!strncmp(command, buffer, sizeof(command) - 1)) {
> + p = buffer + 16;
> + if(sscanf(p,"%16s",devname)>0) {
> + dev = dev_get_by_name(net,devname);
> + if (dev) {
> + if (dev->get_stats) {
> + struct net_device_stats *stats =
> + dev->get_stats(dev);
> + memset(stats,0,
> + sizeof(struct
> net_device_stats));
> + }
> + dev_put(dev);
> + }
Shouldn't this return an error if the device doesn't exist?
> + }
> + }
> +
> + /*
> + * convert success returns so that we return the
> + * number of bytes consumed.
> + */
> + if (!err)
> + err = length;
This won't work; the function updates err unconditionally further up!
> +
> + put_net(net);
> +
> + out:
> + free_page((unsigned long)buffer);
> + return err;
> +}
> +
> static void *softnet_seq_start(struct seq_file *seq, loff_t *pos)
> @@ -2498,2 +2576,3 @@
> .read = seq_read,
> + .write = proc_net_dev_write,
> .llseek = seq_lseek,
The context for this chunk seems to be too short.
There are a few formatting oddities; checkpatch.pl will point them out.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
next prev parent reply other threads:[~2008-05-18 0:31 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <482DA5B6.1020606@sngx.net>
2008-05-16 20:00 ` [PATCH updated] net: add ability to clear per-interface network statistics via procfs David Miller
2008-05-16 20:09 ` Rick Jones
2008-05-17 15:06 ` James Cammarata
[not found] ` <482DB46A.8020103@cosmosbay.com>
2008-05-16 20:03 ` David Miller
[not found] ` <482EF192.4070707@sngx.net>
2008-05-17 21:41 ` Eric Dumazet
2008-05-17 22:49 ` James Cammarata
2008-05-18 0:31 ` Ben Hutchings [this message]
2008-05-18 1:43 ` Patrick McHardy
2008-05-18 5:09 ` James Cammarata
2008-05-18 11:27 ` Ben Hutchings
2008-05-29 1:45 ` [PATCH] net: add ability to clear stats via ethtool - e1000/pcnet32 James Cammarata
2008-05-29 2:08 ` James Cammarata
2008-05-29 5:11 ` Andrew Morton
2008-05-29 12:34 ` James Cammarata
2008-05-29 14:45 ` Alan Cox
2008-05-29 17:15 ` James Cammarata
2008-05-29 20:50 ` David Miller
2008-05-29 21:18 ` James Cammarata
2008-05-30 19:12 ` Bill Fink
2008-05-30 22:14 ` Rick Jones
2008-05-31 1:09 ` Bill Fink
2008-05-31 2:41 ` Stephen Hemminger
2008-05-31 4:47 ` Bill Fink
2008-05-31 12:11 ` Alan Cox
2008-05-31 23:57 ` Bill Fink
2008-06-01 1:46 ` Ben Hutchings
2008-06-01 20:46 ` Bill Fink
2008-06-01 22:29 ` Ben Hutchings
2008-06-02 3:55 ` Bill Fink
2008-06-02 5:39 ` David Miller
2008-06-02 15:41 ` Bill Fink
2008-06-02 4:50 ` Glen Turner
2008-06-02 16:10 ` Bill Fink
2008-06-03 12:28 ` James Cammarata
2008-06-03 12:35 ` Ben Hutchings
2008-06-03 14:46 ` Alan Cox
2008-06-04 3:05 ` James Cammarata
2008-05-29 14:48 ` Chris Friesen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080518003104.GK28241@solarflare.com \
--to=bhutchings@solarflare$(echo .)com \
--cc=dada1@cosmosbay$(echo .)com \
--cc=jimi@sngx$(echo .)net \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox