From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Arnd Bergmann <arnd@arndb.de>, Martin Wilck <mwilck@suse.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-kernel@vger.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Subject: Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy()
Date: Tue, 21 Aug 2018 18:50:55 +0900 [thread overview]
Message-ID: <20180821095055.GA400@jagdpanzerIV> (raw)
In-Reply-To: <0e06858f-3625-692a-582d-d828a3cc3ebe@rasmusvillemoes.dk>
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
next prev parent reply other threads:[~2018-08-21 9:51 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180821095055.GA400@jagdpanzerIV \
--to=sergey.senozhatsky.work@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mwilck@suse.com \
--cc=sergey.senozhatsky@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).