* [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() @ 2018-08-21 6:24 Sergey Senozhatsky 2018-08-21 7:59 ` Rasmus Villemoes 0 siblings, 1 reply; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-21 6:24 UTC (permalink / raw) To: Andrew Morton Cc: Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky In sysfs ->store() callbacks we usually need to remember that a supplied sysfs input string might or might not contain whitespaces and a trailing new-line symbol, which we need to take care of. Examples: echo "newline" > /sys/.../attr echo -n "no_newine" > /sys/.../attr That's why a typical sysfs ->store() should do something like below when it copies a string: ssize_t FOO_store(struct device *dev, ....) { .... strlcpy(value, buf, MAX_SZ); sz = strlen(value); if (sz > 0 && value[sz - 1] == '\n') value[sz - 1] = 0x00; ... } or use a sysfs-string friendly strcmp() function when comparing a given string to a pre-defined one: ssize_t FOO_store(struct device *dev, ....) { ... if (sysfs_streq(buf, "normal")) mode = m_normal; ... } Per Andrew Morton: : There's a LOT of code which does basically-the-same-thing with sysfs : input. And a lot of it misses things, such as leading whitespace. : Passing all this through helpers would provide consistency as well : as code-size reductions, improved reviewability, etc. This patch introduces two such helpers - sysfs_strncpy() and sysfs_strlcpy(), which, basically, do what strncpy() and strlcpy() do, but additionally they remove leading and trailing white-spaces and tailing new-line symbols. So a FOO_store() example which was posted above may be rewritten to: ssize_t FOO_store(struct device *dev, ....) { .... sysfs_strlcpy(value, buf, MAX_SZ); ... } Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> Suggested-by: Andrew Morton <akpm@linux-foundation.org> --- include/linux/string.h | 3 +++ lib/string.c | 51 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/include/linux/string.h b/include/linux/string.h index 4a5a0eb7df51..62b08bbc3ada 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -175,6 +175,9 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp); extern void argv_free(char **argv); extern bool sysfs_streq(const char *s1, const char *s2); +extern char *sysfs_strncpy(char *dest, const char *src, size_t count); +extern size_t sysfs_strlcpy(char *dest, const char *src, size_t size); + extern int kstrtobool(const char *s, bool *res); static inline int strtobool(const char *s, bool *res) { diff --git a/lib/string.c b/lib/string.c index 2c0900a5d51a..e81b1be00796 100644 --- a/lib/string.c +++ b/lib/string.c @@ -631,6 +631,57 @@ bool sysfs_streq(const char *s1, const char *s2) } EXPORT_SYMBOL(sysfs_streq); +/** + * sysfs_strncpy - Trim a length-limited C-string (wgutesoaces and a trailing + * newline symbol) and copy into a buffer + * @dest: Where to copy the string to + * @src: Where to copy the string from + * @count: The maximum number of bytes to copy + * + * A wrapper around strncpy(). + * + */ +char *sysfs_strncpy(char *dest, const char *src, size_t count) +{ + char *c; + + strncpy(dest, skip_spaces(src), count); + + c = dest + count - 1; + while (c >= dest && (isspace(*c) || *c == '\n' || *c == '\0')) { + *c = '\0'; + c--; + } + return dest; +} +EXPORT_SYMBOL(sysfs_strncpy); + +/** + * sysfs_strlcpy - Trim a C-string (whitespaces and a trailing newline symbol) + * and copy it into a sized buffer + * @dest: Where to copy the string to + * @src: Where to copy the string from + * @size: size of destination buffer + * + * A wrapper around strlcpy(). + * + */ +size_t sysfs_strlcpy(char *dest, const char *src, size_t size) +{ + size_t ret; + char *c; + + ret = strlcpy(dest, skip_spaces(src), size); + + size = strlen(dest); + c = dest + size - 1; + while (c >= dest && (isspace(*c) || *c == '\n')) + c--; + *(c + 1) = '\0'; + return ret; +} +EXPORT_SYMBOL(sysfs_strlcpy); + /** * match_string - matches given string in an array * @array: array of strings -- 2.18.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 6:24 [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() Sergey Senozhatsky @ 2018-08-21 7:59 ` Rasmus Villemoes 2018-08-21 9:50 ` Sergey Senozhatsky 0 siblings, 1 reply; 11+ messages in thread From: Rasmus Villemoes @ 2018-08-21 7:59 UTC (permalink / raw) To: Sergey Senozhatsky, Andrew Morton Cc: Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky On 2018-08-21 08:24, Sergey Senozhatsky wrote: > +/** > + * sysfs_strncpy - Trim a length-limited C-string (wgutesoaces and a trailing > + * newline symbol) and copy into a buffer > + * @dest: Where to copy the string to > + * @src: Where to copy the string from > + * @count: The maximum number of bytes to copy > + * > + * A wrapper around strncpy(). > + * > + */ > +char *sysfs_strncpy(char *dest, const char *src, size_t count) > +{ > + char *c; > + > + strncpy(dest, skip_spaces(src), count); I'd like to see where and how you'd use this, but I'm very skeptical of count being used both for the size of the dest buffer as well as an essentially random argument to strncpy - if count is also the maximum number of bytes to read from the src, you'd need to take the skip_spaces() into account, because there are not count bytes left after that... And if src is not necessarily nul-terminated, skip_spaces() by itself is wrong. Moreover, I don't think we should add more users or wrappers for strncpy - I highly doubt the sysfs users you have in mind want the "fill the rest of the buffer with '\0'" nor the "not enough room for a terminating '\0'? Oh well, what could possibly go wrong" semantics. > + c = dest + count - 1; > + while (c >= dest && (isspace(*c) || *c == '\n' || *c == '\0')) { nit: '\n' certainly already passes the isspace() test. > + *c = '\0'; > + c--; > + } > + return dest; > +} > +EXPORT_SYMBOL(sysfs_strncpy); > + > +/** > + * sysfs_strlcpy - Trim a C-string (whitespaces and a trailing newline symbol) > + * and copy it into a sized buffer > + * @dest: Where to copy the string to > + * @src: Where to copy the string from > + * @size: size of destination buffer > + * > + * A wrapper around strlcpy(). > + * > + */ > +size_t sysfs_strlcpy(char *dest, const char *src, size_t size) > +{ > + size_t ret; > + char *c; > + > + ret = strlcpy(dest, skip_spaces(src), size); > + > + size = strlen(dest); > + c = dest + size - 1; > + while (c >= dest && (isspace(*c) || *c == '\n')) > + c--; > + *(c + 1) = '\0'; > + return ret; > +} What exactly is the return value? The string length of the src after skipping leading whitespace? What does that really have to do with the string now in dest, and what is the user supposed to do with it? A more useful return value would either be "the length of the string now in dest", or some sort of indicator that the input was truncated, if that is ever possible. I think you're too focused on making wrappers around str[ln]cpy preserving parts of those functions' API. Instead, try to figure out what sysfs users actually want, name the functions after that, and then whether they use strncpy or sprintf or strscpy internally is completely irrelevant. Maybe int strcpy_trim(char *dst, size_t dstsize, const char *src, size_t srcsize) - copy (potentially not '\0'-terminated) src to dst, trimming leading and trailing whitespace. dstsize must be positive, and dst is guaranteed to be '\0'-terminated. Returns the length of the string now in dst, or -EOVERFLOW if some none-whitespace character was chopped. would cover all use cases? Rasmus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 7:59 ` Rasmus Villemoes @ 2018-08-21 9:50 ` Sergey Senozhatsky 2018-08-21 9:54 ` Sergey Senozhatsky ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-21 9:50 UTC (permalink / raw) To: Rasmus Villemoes Cc: Sergey Senozhatsky, Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky Hi Rasmus, On (08/21/18 09:59), Rasmus Villemoes wrote: > > +char *sysfs_strncpy(char *dest, const char *src, size_t count) > > +{ > > + char *c; > > + > > + strncpy(dest, skip_spaces(src), count); > > I'd like to see where and how you'd use this, but I'm very skeptical of > count being used both for the size of the dest buffer as well as an > essentially random argument to strncpy - if count is also the maximum > number of bytes to read from the src, you'd need to take the > skip_spaces() into account, because there are not count bytes left after > that... > And if src is not necessarily nul-terminated, skip_spaces() by > itself is wrong. 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? > Moreover, I don't think we should add more users or wrappers for strncpy > - I highly doubt the sysfs users you have in mind want the "fill the > rest of the buffer with '\0'" nor the "not enough room for a terminating > '\0'? Oh well, what could possibly go wrong" semantics. The reason I added both strncpy() and strlcpy() was that there are lots of sysfs ->store() callbacks which use strncpy(). E.g. channel_dimm_label_store() dimmdev_label_store() pmbus_add_label() axp20x_store_attr() cmdline_store() and so on and on. > > + c = dest + count - 1; > > + while (c >= dest && (isspace(*c) || *c == '\n' || *c == '\0')) { > > nit: '\n' certainly already passes the isspace() test. Hah, indeed, it should. "\n" & 0x20 must be positive. Andrew had the same comment. But I didn't check what actually isspace() was doing, until now. > > +size_t sysfs_strlcpy(char *dest, const char *src, size_t size) > > +{ > > + size_t ret; > > + char *c; > > + > > + ret = strlcpy(dest, skip_spaces(src), size); > > + > > + size = strlen(dest); > > + c = dest + size - 1; > > + while (c >= dest && (isspace(*c) || *c == '\n')) > > + c--; > > + *(c + 1) = '\0'; > > + return ret; > > +} > > What exactly is the return value? Honestly, I didn't think about it. I wasn't sure if we want to return anything from this function and from sysfs_strncpy(). I glanced through a number of ->store() callbacks, and it seems that mostly people don't bother to check strlcpy() return value at all. > A more useful return value would either be "the length of the string > now in dest", or some sort of indicator that the input was truncated, > if that is ever possible. Agreed. > I think you're too focused on making wrappers around str[ln]cpy > preserving parts of those functions' API. Instead, try to figure out > what sysfs users actually want, name the functions after that, and then > whether they use strncpy or sprintf or strscpy internally is completely > irrelevant. Going point, that's why the patch is in RFC stage: to figure out what do we actually want. > int strcpy_trim(char *dst, size_t dstsize, const char *src, size_t > srcsize) - copy (potentially not '\0'-terminated) src to dst, trimming > leading and trailing whitespace. dstsize must be positive, and dst is > guaranteed to be '\0'-terminated. Returns the length of the string now > in dst, or -EOVERFLOW if some none-whitespace character was chopped. > > would cover all use cases? I like it in general. Sounds like a plan to me. Maybe the "-EOVERFLOW if some none-whitespace character was chopped" part can be changed: if we would trim leading and trailing whitespaces before we copy a string then only valid input chars can get chopped. -ss ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 9:50 ` Sergey Senozhatsky @ 2018-08-21 9:54 ` Sergey Senozhatsky 2018-08-21 11:44 ` Sergey Senozhatsky ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-21 9:54 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Rasmus Villemoes, Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky On (08/21/18 18:50), Sergey Senozhatsky wrote: > > Going point, that's why the patch is in RFC stage: to figure out > what do we actually want. s/Going/Good/ don't know how did that happen. -ss ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 9:50 ` Sergey Senozhatsky 2018-08-21 9:54 ` Sergey Senozhatsky @ 2018-08-21 11:44 ` Sergey Senozhatsky 2018-08-21 12:00 ` Andy Shevchenko 2018-08-21 12:43 ` Rasmus Villemoes 2018-08-21 13:57 ` Greg Kroah-Hartman 3 siblings, 1 reply; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-21 11:44 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky, Sergey Senozhatsky On (08/21/18 18:50), Sergey Senozhatsky wrote: > > > int strcpy_trim(char *dst, size_t dstsize, const char *src, size_t > > srcsize) - copy (potentially not '\0'-terminated) src to dst, trimming > > leading and trailing whitespace. dstsize must be positive, and dst is > > guaranteed to be '\0'-terminated. Returns the length of the string now > > in dst, or -EOVERFLOW if some none-whitespace character was chopped. > > > > would cover all use cases? > Something like below? Not tested, since we are still in "is this what we want" phase. Returning the length of dst/-EOVERFLOW is a bit inconvenient, because "the length" forces us to have size_t return, which is unsigned. This one returns a number of non-NULL bytes, which were copied to dst, or 0. --- size_t strcpy_trim(char *dst, size_t dstsz, const char *src, size_t srcsz) { const char *end; size_t ret = 0; size_t len = 0; if (!dstsz || !srcsz) goto out; end = src + srcsz - 1; while (end >= src && isspace(*end)) end--; end++; while (src < end && isspace(*src)) src++; len = (src >= end) ? 0 : end - src; if (!len) goto out; ret = len; if (len >= dstsz) len = dstsz - 1; memcpy(dst, src, len); out: dst[len] = '\0'; return ret == len ? ret : -EOVERFLOW; } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 11:44 ` Sergey Senozhatsky @ 2018-08-21 12:00 ` Andy Shevchenko 2018-08-22 0:32 ` Sergey Senozhatsky 0 siblings, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2018-08-21 12:00 UTC (permalink / raw) To: Sergey Senozhatsky, Rasmus Villemoes Cc: Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, linux-kernel, Sergey Senozhatsky On Tue, 2018-08-21 at 20:44 +0900, Sergey Senozhatsky wrote: > Something like below? Not tested, since we are still in "is this > what we want" phase. > Returning the length of dst/-EOVERFLOW is a bit inconvenient, because > "the length" forces us to have size_t return, which is unsigned. We have for ages ssize_t to workaround that. > size_t strcpy_trim(char *dst, size_t dstsz, const char *src, size_t > srcsz) > { > const char *end; > size_t ret = 0; > size_t len = 0; > > if (!dstsz || !srcsz) > goto out; > > end = src + srcsz - 1; > while (end >= src && isspace(*end)) > end--; > end++; > while (src < end && isspace(*src)) > src++; > len = (src >= end) ? 0 : end - src; > if (!len) > goto out; > > ret = len; > if (len >= dstsz) > len = dstsz - 1; > memcpy(dst, src, len); > out: > dst[len] = '\0'; > return ret == len ? ret : -EOVERFLOW; > } Wouldn't be better to split out something like strnstrip() out of strim(), with simultaneous changes to strim(), strstrip(), and use it here? -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 12:00 ` Andy Shevchenko @ 2018-08-22 0:32 ` Sergey Senozhatsky 0 siblings, 0 replies; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-22 0:32 UTC (permalink / raw) To: Andy Shevchenko Cc: Sergey Senozhatsky, Rasmus Villemoes, Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, linux-kernel, Sergey Senozhatsky On (08/21/18 15:00), Andy Shevchenko wrote: > > Returning the length of dst/-EOVERFLOW is a bit inconvenient, because > > "the length" forces us to have size_t return, which is unsigned. > > We have for ages ssize_t to workaround that. OK. [..] > Wouldn't be better to split out something like > > strnstrip() out of strim(), with simultaneous changes to strim(), > strstrip(), and use it here? Maybe yes, maybe not. strim() modifies the original string right after it's done moving the end pointer. We can't do that in strcpy_trim() and need to keep the original source string. So probably these two functions don't have that much of a common code after all. -ss ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 9:50 ` Sergey Senozhatsky 2018-08-21 9:54 ` Sergey Senozhatsky 2018-08-21 11:44 ` Sergey Senozhatsky @ 2018-08-21 12:43 ` Rasmus Villemoes 2018-08-22 5:13 ` Sergey Senozhatsky 2018-08-21 13:57 ` Greg Kroah-Hartman 3 siblings, 1 reply; 11+ messages in thread From: Rasmus Villemoes @ 2018-08-21 12:43 UTC (permalink / raw) To: Sergey Senozhatsky, Rasmus Villemoes Cc: Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky On 2018-08-21 11:50, Sergey Senozhatsky wrote: > Hi Rasmus, > > On (08/21/18 09:59), Rasmus Villemoes wrote: >>> +char *sysfs_strncpy(char *dest, const char *src, size_t count) >>> +{ >>> + char *c; >>> + >>> + strncpy(dest, skip_spaces(src), count); >> >> I'd like to see where and how you'd use this, but I'm very skeptical of >> count being used both for the size of the dest buffer as well as an >> essentially random argument to strncpy - if count is also the maximum >> number of bytes to read from the src, you'd need to take the >> skip_spaces() into account, because there are not count bytes left after >> that... >> And if src is not necessarily nul-terminated, skip_spaces() by >> itself is wrong. > > I think that sysfs input is always properly NULL-terminated. It may or > may not contain \n, but \0 is expected to be there. Maybe. But it shouldn't hurt to make it accept a src size (the caller can always pass -1 is he's sure the input is nul-terminated) - I'm thinking a new interface might also have uses outside sysfs. >> Moreover, I don't think we should add more users or wrappers for strncpy >> - I highly doubt the sysfs users you have in mind want the "fill the >> rest of the buffer with '\0'" nor the "not enough room for a terminating >> '\0'? Oh well, what could possibly go wrong" semantics. > > The reason I added both strncpy() and strlcpy() was that there are lots > of sysfs ->store() callbacks which use strncpy(). > > E.g. > channel_dimm_label_store() > dimmdev_label_store() > pmbus_add_label() > axp20x_store_attr() > cmdline_store() Hm, do any of those actually want the skipping of leading whitespace? pmbus_add_label doesn't even seem to kill a trailing newline? It's probably hard to create one interface that will work for all cases. Some are happy with truncating the user input, some refuse to do the copy if truncation might happen, some probably do the copy (clobbering what was there) but then detect the truncation somehow and return EINVAL or whatever. Also, some copy to a pre-existing buffer, while others kmalloc() a buffer and populate it with strncpy. The latter would probably be better served using kstrndup or a small variant. >> I think you're too focused on making wrappers around str[ln]cpy >> preserving parts of those functions' API. Instead, try to figure out >> what sysfs users actually want, name the functions after that, and then >> whether they use strncpy or sprintf or strscpy internally is completely >> irrelevant. > > [Good] point, that's why the patch is in RFC stage: to figure out > what do we actually want. Dunno, looking at your examples above I'm a bit more skeptical that there is a lot of common ground. But of course there are hundreds of other store callbacks, so surely one can simplify some with a new helper. Rasmus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 12:43 ` Rasmus Villemoes @ 2018-08-22 5:13 ` Sergey Senozhatsky 0 siblings, 0 replies; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-22 5:13 UTC (permalink / raw) To: Rasmus Villemoes Cc: Sergey Senozhatsky, Andrew Morton, Arnd Bergmann, Martin Wilck, Greg Kroah-Hartman, Andy Shevchenko, linux-kernel, Sergey Senozhatsky Hi, On (08/21/18 14:43), Rasmus Villemoes 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. > > Maybe. But it shouldn't hurt to make it accept a src size (the caller > can always pass -1 is he's sure the input is nul-terminated) - I'm > thinking a new interface might also have uses outside sysfs. Makes sense. > > E.g. > > channel_dimm_label_store() > > dimmdev_label_store() > > pmbus_add_label() > > axp20x_store_attr() > > cmdline_store() > > Hm, do any of those actually want the skipping of leading whitespace? Hm, that's a good question. I'd probably say "mostly yes". The input usually goes to strcmp/strcpy/strtol/etc. But we don't have to trim leading whitespaces and can just let the driver error out [hopefully the driver has a well tested error-out path]. > pmbus_add_label doesn't even seem to kill a trailing newline? Sorry, I think pmbus_add_label() made it to the list by mistake. It doesn't appear to be a ->store() callback. > It's probably hard to create one interface that will work for all cases. Agreed, that's why I targeted sysfs input data only ;) > Dunno, looking at your examples above I'm a bit more skeptical that > there is a lot of common ground. But of course there are hundreds of > other store callbacks, so surely one can simplify some with a new helper. Yep. -ss ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 9:50 ` Sergey Senozhatsky ` (2 preceding siblings ...) 2018-08-21 12:43 ` Rasmus Villemoes @ 2018-08-21 13:57 ` Greg Kroah-Hartman 2018-08-22 4:58 ` Sergey Senozhatsky 3 siblings, 1 reply; 11+ messages in thread From: Greg Kroah-Hartman @ 2018-08-21 13:57 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Rasmus Villemoes, Andrew Morton, Arnd Bergmann, Martin Wilck, Andy Shevchenko, linux-kernel, Sergey Senozhatsky On Tue, Aug 21, 2018 at 06:50:55PM +0900, Sergey Senozhatsky wrote: > Hi Rasmus, > > On (08/21/18 09:59), Rasmus Villemoes wrote: > > > +char *sysfs_strncpy(char *dest, const char *src, size_t count) > > > +{ > > > + char *c; > > > + > > > + strncpy(dest, skip_spaces(src), count); > > > > I'd like to see where and how you'd use this, but I'm very skeptical of > > count being used both for the size of the dest buffer as well as an > > essentially random argument to strncpy - if count is also the maximum > > number of bytes to read from the src, you'd need to take the > > skip_spaces() into account, because there are not count bytes left after > > that... > > And if src is not necessarily nul-terminated, skip_spaces() by > > itself is wrong. > > 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. You can be very strict in the SINGLE_VALUE that you accept in your sysfs file. And yes, sysfs files are single values, if you want to do more than that, you need to use something else, so I really do not understand what problem you are trying to solve here. 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. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() 2018-08-21 13:57 ` Greg Kroah-Hartman @ 2018-08-22 4:58 ` Sergey Senozhatsky 0 siblings, 0 replies; 11+ messages in thread From: Sergey Senozhatsky @ 2018-08-22 4:58 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Sergey Senozhatsky, Rasmus Villemoes, Andrew Morton, Arnd Bergmann, Martin Wilck, Andy Shevchenko, linux-kernel, Sergey Senozhatsky 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-08-22 5:13 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-21 6:24 [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() Sergey Senozhatsky 2018-08-21 7:59 ` Rasmus Villemoes 2018-08-21 9:50 ` Sergey Senozhatsky 2018-08-21 9:54 ` Sergey Senozhatsky 2018-08-21 11:44 ` Sergey Senozhatsky 2018-08-21 12:00 ` Andy Shevchenko 2018-08-22 0:32 ` Sergey Senozhatsky 2018-08-21 12:43 ` Rasmus Villemoes 2018-08-22 5:13 ` Sergey Senozhatsky 2018-08-21 13:57 ` Greg Kroah-Hartman 2018-08-22 4:58 ` Sergey Senozhatsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).