svn commit: r253048 - in head/sys/ofed: drivers/net/mlx4 include/linux

[John Baldwin]

On Monday, July 08, 2013 6:13:22 pm Garrett Cooper wrote:
> On Jul 8, 2013, at 2:25 PM, John Baldwin wrote:
> 
> > Author: jhb
> > Date: Mon Jul  8 21:25:12 2013
> > New Revision: 253048
> > URL: http://svnweb.freebsd.org/changeset/base/253048
> > 
> > Log:
> >  Allow mlx4 devices to switch from Ethernet to Infiniband (and vice 
versa):
> >  - Fix sysctl wrapper for sysfs attributes to properly handle new string
> >    values similar to sysctl_handle_string() (only copyin the user's
> >    supplied length and nul-terminate the string).
> >  - Don't check for a trailing newline when evaluating the desired 
operating
> >    mode of a mlx4 device.
> > 
> >  PR:		kern/179999
> >  Submitted by:	Shahar Klein <shahark at mellanox.com>
> >  MFC after:	1 week
> 
> Was there an issue with the patch I submitted via 
http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/174213 (basically solving the 
same problem, but only with the sysfs <-> sysctl(9) handler)? I was of the 
impression that sysfs on Linux always added on trailing newlines (but I could 
be wrong because I haven't used Linux at a dev level for ages). Thanks!

I hadn't seen it.  I had wondered if the '\n' issue was a generic sysfs thing.
It sounds like it is and I'd be happy to revert the mlx4 change and alter the
sysfs bits to manage the newline directly if that is more appropriate.  I'd
also like this to use sysctl_handle_string() if at all possible.  Are you in
a position to test patches still?

If so, maybe give this a whirl.  It's similar to yours except it uses
sysctl_handle_string() and strlcat() rather than continuing to do things
by hand.  It also outputs an empty string to userland if the attribute
doesn't have a show method (your version would never pass out an old
string in that case unlike the original code).

Index: drivers/net/mlx4/main.c
===================================================================
--- drivers/net/mlx4/main.c	(revision 253096)
+++ drivers/net/mlx4/main.c	(working copy)
@@ -479,11 +479,11 @@
 	int i;
 	int err = 0;
 
-	if (!strcmp(buf, "ib"))
+	if (!strcmp(buf, "ib\n"))
 		info->tmp_type = MLX4_PORT_TYPE_IB;
-	else if (!strcmp(buf, "eth"))
+	else if (!strcmp(buf, "eth\n"))
 		info->tmp_type = MLX4_PORT_TYPE_ETH;
-	else if (!strcmp(buf, "auto"))
+	else if (!strcmp(buf, "auto\n"))
 		info->tmp_type = MLX4_PORT_TYPE_AUTO;
 	else {
 		mlx4_err(mdev, "%s is not supported port type\n", buf);
Index: include/linux/sysfs.h
===================================================================
--- include/linux/sysfs.h	(revision 253096)
+++ include/linux/sysfs.h	(working copy)
@@ -81,37 +81,35 @@
 
 	kobj = arg1;
 	attr = (struct attribute *)arg2;
-	buf = (void *)get_zeroed_page(GFP_KERNEL);
-	len = 1;	/* Copy out a NULL byte at least. */
 	if (kobj->ktype == NULL || kobj->ktype->sysfs_ops == NULL)
 		return (ENODEV);
-	ops = kobj->ktype->sysfs_ops;
+	buf = (void *)get_zeroed_page(GFP_KERNEL);
 	if (buf == NULL)
 		return (ENOMEM);
+	ops = kobj->ktype->sysfs_ops;
 	if (ops->show) {
 		len = ops->show(kobj, attr, buf);
 		/*
-		 * It's valid not to have a 'show' so we just return 1 byte
-		 * of NULL.
+		 * It's valid to not have a 'show' so just return an
+		 * empty string.
 	 	 */
 		if (len < 0) {
 			error = -len;
-			len = 1;
 			if (error != EIO)
 				goto out;
 		}
+
+		/* Trim trailing newline. */
+		len--;
+		buf[len] = '\0';
 	}
-	error = SYSCTL_OUT(req, buf, len);
-	if (error || !req->newptr || ops->store == NULL)
+
+	/* Leave one trailing byte to append a newline. */
+	error = sysctl_handle_string(oidp, buf, PAGE_SIZE - 1, req);
+	if (error != 0 || req->newptr == NULL || ops->store == NULL)
 		goto out;
-	len = req->newlen - req->newidx;
-	if (len >= PAGE_SIZE)
-		error = EINVAL;
-	else 
-		error = SYSCTL_IN(req, buf, len);
-	if (error)
-		goto out;
-	((char *)buf)[len] = '\0';
+	len = strlcat(buf, "\n", PAGE_SIZE);
+	KASSERT(len < PAGE_SIZE, ("new attribute truncated"));
 	len = ops->store(kobj, attr, buf, len);
 	if (len < 0)
 		error = -len;

-- 
John Baldwin