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