Messages in this thread Patch in this message |  | | Date | Wed, 22 Aug 2018 13:58:23 +0900 | From | Sergey Senozhatsky <> | 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
|  |