Thread (34 messages) 34 messages, 12 authors, 2008-06-04

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)

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-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!
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help