Messages in this thread |  | | Subject | Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() | From | Rasmus Villemoes <> | Date | Tue, 21 Aug 2018 09:59:42 +0200 |
| |
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
|  |