Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs
From: Ben Hutchings <hidden>
Date: 2008-05-18 00:31:34
Also in:
lkml
Possibly related (same subject, not in this thread)
- 2008-05-17 · Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs · James Cammarata <hidden>
- 2008-05-16 · Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs · Rick Jones <hidden>
- 2008-05-16 · Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs · David Miller <davem@davemloft.net>
- 2008-05-16 · Re: [PATCH updated] net: add ability to clear per-interface network statistics via procfs · David Miller <davem@davemloft.net>
James Cammarata wrote:
quoted hunk ↗ jump to hunk
quoted
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-1717: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!
quoted hunk ↗ jump to hunk
+ + 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.