Messages in this thread |  | | Date | Tue, 21 Aug 2018 18:50:55 +0900 | From | Sergey Senozhatsky <> | Subject | Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy() |
| |
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
|  |