Re: [RFC][PATCH] lib/string: introduce sysfs_strncpy() and sysfs_strlcpy()

[Rasmus Villemoes]

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