Message-ID: <20180822045823.GA6876@jagdpanzerIV>
Date:   Wed, 22 Aug 2018 13:58:23 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>, Martin Wilck <mwilck@...e.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and
 sysfs_strlcpy()

Hello Greg,

On (08/21/18 15:57), Greg Kroah-Hartman wrote:
> > I think that sysfs input is always properly NULL-terminated. It may or
> > may not contain \n, but \0 is expected to be there. Am I wrong?
> 
> sysfs data is always null terminated.
> 
> What exactly are you trying to do here?  If a user sends you crappy data
> in a sysfs file (like leading or trailing whitespace), well, you can
> always just error out, no problem.

So what we are thinking about is sort of clean up / unification of the
way ->store() callbacks handle sysfs input. They all have to do the
same things for "corner case" handling and many of them forget to handle
some specific cases; or have to come up with extra code; or are not aware
of the existing API; etc.

For instance, let's look at 325c4b3b81027068 [well, a small part of]

---

@@ -245,7 +239,7 @@ static ssize_t pm_qos_resume_latency_store(struct device *dev,
 
                if (value == 0)
                        value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
-       } else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
+       } else if (sysfs_streq(buf, "n/a")) {
                value = 0;
        } else {
                return -EINVAL;
@@ -285,9 +279,9 @@ static ssize_t pm_qos_latency_tolerance_store(struct device *dev,
                if (value < 0)
                        return -EINVAL;
        } else {
-               if (!strcmp(buf, "auto") || !strcmp(buf, "auto\n"))
+               if (sysfs_streq(buf, "auto"))
                        value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
-               else if (!strcmp(buf, "any") || !strcmp(buf, "any\n"))
+               else if (sysfs_streq(buf, "any"))
                        value = PM_QOS_LATENCY_ANY;
                else
                        return -EINVAL;
@@ -342,20 +336,12 @@ static ssize_t
 wake_store(struct device * dev, struct device_attribute *attr,
        const char * buf, size_t n)
 {
-       char *cp;
-       int len = n;
-
        if (!device_can_wakeup(dev))
                return -EINVAL;
 
-       cp = memchr(buf, '\n', n);
-       if (cp)
-               len = cp - buf;
-       if (len == sizeof _enabled - 1
-                       && strncmp(buf, _enabled, sizeof _enabled - 1) == 0)
+       if (sysfs_streq(buf, _enabled))
                device_set_wakeup_enable(dev, 1);
-       else if (len == sizeof _disabled - 1
-                       && strncmp(buf, _disabled, sizeof _disabled - 1) == 0)
+       else if (sysfs_streq(buf, _disabled))
                device_set_wakeup_enable(dev, 0);
        else
                return -EINVAL;
---

There was quite a bit of code. But these things still can and do happen;
Andy tweaked the existing code only.

So what we are looking at is a way which would let us to make that part
of drivers to be simpler and less fragile, perhaps.

> Please always post a user of your new api when you make stuff like this
> otherwise we do not know how it is used, or even why you are adding it.

Sure, I agree. There is no API proposal yet; so I gave a simple example
in the commit message and didn't bother to convert any of the existing
users.

I'm not even sure yet if we want to have a new API. The sort of a
root cause [it seems so] here is that sysfs input data has irregular
format. That's why we have irregular handling of

Either in a form of

		if (!strcmp(buf, "auto") || !strcmp(buf, "auto\n"))
			...
or in a form of

		if (sz > 0 && value[sz - 1] == '\n')
			value[sz - 1] = 0x00;
		if (!strcmp(value, "auto"))

and so on.

So may be, instead of new API which was meant to help make sysfs data
look uniform, we can do tweaks to sysfs and pass to ->store() callbacks
data which already has no trailing newline and whitespaces. IOW, make it
uniform within sysfs. Then we can remove a bunch of code from the existing
drivers and make it easier for future drivers.
So sysfs could do strim-ing, etc. and ->store() would always receive data
which can be directly used as strcmp/strcpy/etc input. Because this is what
people want to do after all; but they learn at some point that they can't
and there are newline symbols, etc. to take care of.

What do you think? A new API is probably safer option here; but then,
again, people can forget to use it, or be unaware of it.

	-ss

Powered by blists - more mailing lists