| Message ID | 1391546631-7715-2-git-send-email-sebastian.capella@linaro.org |
|---|---|
| State | New |
| Headers | show |
Quoting Andrew Morton (2014-02-05 13:50:52) > On Tue, 4 Feb 2014 12:43:49 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote: > > > kstrdup_trimnl creates a duplicate of the passed in > > null-terminated string. If a trailing newline is found, it > > is removed before duplicating. This is useful for strings > > coming from sysfs that often include trailing whitespace due to > > user input. > > hm, why? I doubt if any caller of this wants to retain leading and/or > trailing spaces and/or tabs. Hi Andrew, I agree the common case doesn't usually need leading or trailing whitespace. Pavel and others pointed out that a valid filename could contain newlines/whitespace at any position. If we allow for this, then it would be incorrect to strip whitespace from the input. Comments also went down the lines that it would be better for the kernel not to second guess what is being passed in. I find stripping the trailing newline to be very useful, and there are many examples of kernel code doing this. I think it would be a mistake to remove this now, and would be confusing for users. A compromise is to strip the final newline only if it's present before the null. This allows the common case of echoing a simple string onto /sys/power/resume, and behaves as expected in that case. A complex string without a trailing newline is also handled by quoting or dding a file onto /sys/power/resume. In the unlikely event a user has trailing newline as part of the input, then adding an additional newline to the end will cover that case. This is not ideal, but it puts the additional burden onto the complex case rather than the common case. Thanks, Sebastian
Quoting Andrew Morton (2014-02-05 15:01:01) > On Wed, 05 Feb 2014 14:55:52 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote: > > > Quoting Andrew Morton (2014-02-05 13:50:52) > > > On Tue, 4 Feb 2014 12:43:49 -0800 Sebastian Capella <sebastian.capella@linaro.org> wrote: > > > > > > > kstrdup_trimnl creates a duplicate of the passed in > > > > null-terminated string. If a trailing newline is found, it > > > > is removed before duplicating. This is useful for strings > > > > coming from sysfs that often include trailing whitespace due to > > > > user input. > > > > > > hm, why? I doubt if any caller of this wants to retain leading and/or > > > trailing spaces and/or tabs. > > > > Hi Andrew, > > > > I agree the common case doesn't usually need leading or trailing whitespace. > > > > Pavel and others pointed out that a valid filename could contain > > newlines/whitespace at any position. > > The number of cases in which we provide the kernel with a filename via > sysfs will be very very small, or zero. > > If we can go through existing code and find at least a few sites which > can usefully employ kstrdup_trimnl() then fine, we have evidence. But > I doubt if we can do that? Hi Andrew, I went through all of the store functions I could find and, though I found a lot of examples handling \n, I found no other examples specifically parsing filenames. Most deal with integers. Those parsing commands often use sysfs_streq or otherwise are doing some custom behavior that wouldn't suit a utility function. For my purposes, it looks like v2 of the patch seems like the best starting point based on all of the feedback I've received. So I'm moving back to a custom solution for parsing this input. Unless someone objects or has comments, I'll post something like the function below. static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) { dev_t res; int len = n; char *name; if (len && buf[len-1] == '\n') len--; name = kstrndup(buf, len, GFP_KERNEL); if (!name) return -ENOMEM; res = name_to_dev_t(name); kfree(name); if (!res) return -EINVAL; lock_system_sleep(); swsusp_resume_device = res; unlock_system_sleep(); printk(KERN_INFO "PM: Starting manual resume from disk\n"); noresume = 0; software_resume(); return n; } Thanks, Sebastian
diff --git a/include/linux/string.h b/include/linux/string.h index ac889c5..e7ec8c0 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -114,6 +114,7 @@ void *memchr_inv(const void *s, int c, size_t n); extern char *kstrdup(const char *s, gfp_t gfp); extern char *kstrndup(const char *s, size_t len, gfp_t gfp); +extern char *kstrdup_trimnl(const char *s, gfp_t gfp); extern void *kmemdup(const void *src, size_t len, gfp_t gfp); extern char **argv_split(gfp_t gfp, const char *str, int *argcp); diff --git a/mm/util.c b/mm/util.c index 808f375..0bab867 100644 --- a/mm/util.c +++ b/mm/util.c @@ -1,6 +1,7 @@ #include <linux/mm.h> #include <linux/slab.h> #include <linux/string.h> +#include <linux/ctype.h> #include <linux/export.h> #include <linux/err.h> #include <linux/sched.h> @@ -63,6 +64,34 @@ char *kstrndup(const char *s, size_t max, gfp_t gfp) EXPORT_SYMBOL(kstrndup); /** + * kstrdup_trimnl - Copy a %NUL terminated string, removing one trailing + * newline if present. + * @s: the string to duplicate + * @gfp: the GFP mask used in the kmalloc() call when allocating memory + * + * Returns an address, which the caller must kfree, containing + * a duplicate of the passed string with a single trailing newline + * removed if present. + */ +char *kstrdup_trimnl(const char *s, gfp_t gfp) +{ + char *buf; + size_t len = strlen(s); + if (len >= 1 && s[len - 1] == '\n') + len--; + + buf = kmalloc_track_caller(len + 1, gfp); + if (!buf) + return NULL; + + memcpy(buf, s, len); + buf[len] = '\0'; + + return buf; +} +EXPORT_SYMBOL(kstrdup_trimnl); + +/** * kmemdup - duplicate region of memory * * @src: memory region to duplicate
kstrdup_trimnl creates a duplicate of the passed in null-terminated string. If a trailing newline is found, it is removed before duplicating. This is useful for strings coming from sysfs that often include trailing whitespace due to user input. Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Rik van Riel <riel@redhat.com> (commit_signer:5/10=50%) Cc: Michel Lespinasse <walken@google.com> Cc: Shaohua Li <shli@kernel.org> Cc: Jerome Marchand <jmarchan@redhat.com> Cc: Mikulas Patocka <mpatocka@redhat.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Joe Perches <joe@perches.com> Cc: David Rientjes <rientjes@google.com> Cc: Alexey Dobriyan <adobriyan@gmail.com> --- include/linux/string.h | 1 + mm/util.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)