Doing single-character substitution on an entire string is open-coded
in a few places, sometimes in a rather suboptimal way. This introduces
a trivial helper, strreplace, for this task along with a few example
conversions.
Andrew, can I get you to take 1/8 through the mm tree? I'm not sure
what the easiest path is for the remaining patches.
Rasmus Villemoes (8):
lib: string: Introduce strreplace
kernel/trace/trace_events_filter.c: Use strreplace
blktrace: use strreplace in do_blk_trace_setup
lib/kobject.c: Use strreplace
drivers/base/core.c: Use strreplace
drivers/md/md.c: Use strreplace
fs/jbd2/journal.c: Use strreplace
fs/ext4/super.c: Use strreplace in ext4_fill_super
drivers/base/core.c | 9 ++++-----
drivers/md/md.c | 4 +---
fs/ext4/super.c | 4 +---
fs/jbd2/journal.c | 10 ++--------
include/linux/string.h | 1 +
kernel/trace/blktrace.c | 6 ++----
kernel/trace/trace_events_filter.c | 5 ++---
lib/kobject.c | 13 +++++--------
lib/string.c | 17 +++++++++++++++++
9 files changed, 35 insertions(+), 34 deletions(-)
--
2.1.3
Strings are sometimes sanitized by replacing a certain character
(often '/') by another (often '!'). In a few places, this is done the
same way Schlemiel the Painter would do it. Others are slightly
smarter but still do multiple strchr() calls. Introduce strreplace()
to do this using a single function call and a single pass over the
string.
One would expect the return value to be one of three things: void, s,
or the number of replacements made. I chose the fourth, returning a
pointer to the end of the string. This is more likely to be useful
(for example allowing the caller to avoid a strlen call).
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: spello fixed, parameters renamed 'old' and 'new' (just so the
kernel doc aligns nicely, and because that's what python -c
'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
caused more bloat), still called strreplace.
include/linux/string.h | 1 +
lib/string.c | 17 +++++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index e40099e585c9..a8d90db9c4b0 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -111,6 +111,7 @@ extern int memcmp(const void *,const void *,__kernel_size_t);
extern void * memchr(const void *,int,__kernel_size_t);
#endif
void *memchr_inv(const void *s, int c, size_t n);
+char *strreplace(char *s, char old, char new);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c
index bb3d4b6993c4..13d1e84ddb80 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -849,3 +849,20 @@ void *memchr_inv(const void *start, int c, size_t bytes)
return check_bytes8(start, value, bytes % 8);
}
EXPORT_SYMBOL(memchr_inv);
+
+/**
+ * strreplace - Replace all occurrences of character in string.
+ * @s: The string to operate on.
+ * @old: The character being replaced.
+ * @new: The character @old is replaced with.
+ *
+ * Returns pointer to the nul byte at the end of @s.
+ */
+char *strreplace(char *s, char old, char new)
+{
+ for (; *s; ++s)
+ if (*s == old)
+ *s = new;
+ return s;
+}
+EXPORT_SYMBOL(strreplace);
--
2.1.3
There's no point in starting over every time we see a ','...
Signed-off-by: Rasmus Villemoes <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
---
v2: same patch, added Ack.
kernel/trace/trace_events_filter.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index ced69da0ff55..a987601d7b43 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -2075,7 +2075,7 @@ struct function_filter_data {
static char **
ftrace_function_filter_re(char *buf, int len, int *count)
{
- char *str, *sep, **re;
+ char *str, **re;
str = kstrndup(buf, len, GFP_KERNEL);
if (!str)
@@ -2085,8 +2085,7 @@ ftrace_function_filter_re(char *buf, int len, int *count)
* The argv_split function takes white space
* as a separator, so convert ',' into spaces.
*/
- while ((sep = strchr(str, ',')))
- *sep = ' ';
+ strreplace(str, ',', ' ');
re = argv_split(GFP_KERNEL, str, count);
kfree(str);
--
2.1.3
Part of the disassembly of do_blk_trace_setup:
231b: e8 00 00 00 00 callq 2320 <do_blk_trace_setup+0x50>
231c: R_X86_64_PC32 strlen+0xfffffffffffffffc
2320: eb 0a jmp 232c <do_blk_trace_setup+0x5c>
2322: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
2328: 48 83 c3 01 add $0x1,%rbx
232c: 48 39 d8 cmp %rbx,%rax
232f: 76 47 jbe 2378 <do_blk_trace_setup+0xa8>
2331: 41 80 3c 1c 2f cmpb $0x2f,(%r12,%rbx,1)
2336: 75 f0 jne 2328 <do_blk_trace_setup+0x58>
2338: 41 c6 04 1c 5f movb $0x5f,(%r12,%rbx,1)
233d: 4c 89 e7 mov %r12,%rdi
2340: e8 00 00 00 00 callq 2345 <do_blk_trace_setup+0x75>
2341: R_X86_64_PC32 strlen+0xfffffffffffffffc
2345: eb e1 jmp 2328 <do_blk_trace_setup+0x58>
Yep, that's right: gcc isn't smart enough to realize that replacing
'/' by '_' cannot change the strlen(), so we call it again and again
(at least when a '/' is found). Even if gcc were that smart, this
construction would still loop over the string twice, once for the
initial strlen() call and then the open-coded loop.
Let's simply use strreplace() instead.
Signed-off-by: Rasmus Villemoes <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
---
v2: same patch, added Ack.
kernel/trace/blktrace.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 483cecfa5c17..4eeae4674b5a 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -439,7 +439,7 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
{
struct blk_trace *old_bt, *bt = NULL;
struct dentry *dir = NULL;
- int ret, i;
+ int ret;
if (!buts->buf_size || !buts->buf_nr)
return -EINVAL;
@@ -451,9 +451,7 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
* some device names have larger paths - convert the slashes
* to underscores for this to work as expected
*/
- for (i = 0; i < strlen(buts->name); i++)
- if (buts->name[i] == '/')
- buts->name[i] = '_';
+ strreplace(buts->name, '/', '_');
bt = kzalloc(sizeof(*bt), GFP_KERNEL);
if (!bt)
--
2.1.3
There's probably not many slashes in the name, but starting over when
we see one feels wrong.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: Original code relied on the const laundering done by strchr; v1
had a corresponding explicit (char*) cast. Avoid this altogether.
lib/kobject.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index 3b841b97fccd..75ee63834fd1 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -257,23 +257,20 @@ static int kobject_add_internal(struct kobject *kobj)
int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
va_list vargs)
{
- const char *old_name = kobj->name;
char *s;
if (kobj->name && !fmt)
return 0;
- kobj->name = kvasprintf(GFP_KERNEL, fmt, vargs);
- if (!kobj->name) {
- kobj->name = old_name;
+ s = kvasprintf(GFP_KERNEL, fmt, vargs);
+ if (!s)
return -ENOMEM;
- }
/* ewww... some of these buggers have '/' in the name ... */
- while ((s = strchr(kobj->name, '/')))
- s[0] = '!';
+ strreplace(s, '/', '!');
+ kfree(kobj->name);
+ kobj->name = s;
- kfree(old_name);
return 0;
}
--
2.1.3
This eliminates a little .text and avoids repeating the strchr call
when we meet a '!' (which will happen at least once).
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: Avoid ugly (char *) cast.
drivers/base/core.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 21d13038534e..dafae6d2f7ac 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1303,12 +1303,11 @@ const char *device_get_devnode(struct device *dev,
return dev_name(dev);
/* replace '!' in the name with '/' */
- *tmp = kstrdup(dev_name(dev), GFP_KERNEL);
- if (!*tmp)
+ s = kstrdup(dev_name(dev), GFP_KERNEL);
+ if (!s)
return NULL;
- while ((s = strchr(*tmp, '!')))
- s[0] = '/';
- return *tmp;
+ strreplace(s, '!', '/');
+ return *tmp = s;
}
/**
--
2.1.3
There's no point in starting over when we meet a '/'. This also
eliminates a stack variable and a little .text.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: no changes.
drivers/md/md.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 27506302eb7a..2ea2f28551c5 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2024,7 +2024,6 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
{
char b[BDEVNAME_SIZE];
struct kobject *ko;
- char *s;
int err;
/* prevent duplicates */
@@ -2070,8 +2069,7 @@ static int bind_rdev_to_array(struct md_rdev *rdev, struct mddev *mddev)
return -EBUSY;
}
bdevname(rdev->bdev,b);
- while ( (s=strchr(b, '/')) != NULL)
- *s = '!';
+ strreplace(b, '/', '!');
rdev->mddev = mddev;
printk(KERN_INFO "md: bind<%s>\n", b);
--
2.1.3
In one case, we eliminate a local variable; in the other a strlen()
call and some .text.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: no changes.
fs/jbd2/journal.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b96bd8076b70..5c187ded12d6 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1137,7 +1137,6 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
{
journal_t *journal = journal_init_common();
struct buffer_head *bh;
- char *p;
int n;
if (!journal)
@@ -1150,9 +1149,7 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
journal->j_blk_offset = start;
journal->j_maxlen = len;
bdevname(journal->j_dev, journal->j_devname);
- p = journal->j_devname;
- while ((p = strchr(p, '/')))
- *p = '!';
+ strreplace(journal->j_devname, '/', '!');
jbd2_stats_proc_init(journal);
n = journal->j_blocksize / sizeof(journal_block_tag_t);
journal->j_wbufsize = n;
@@ -1204,10 +1201,7 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
journal->j_inode = inode;
bdevname(journal->j_dev, journal->j_devname);
- p = journal->j_devname;
- while ((p = strchr(p, '/')))
- *p = '!';
- p = journal->j_devname + strlen(journal->j_devname);
+ p = strreplace(journal->j_devname, '/', '!');
sprintf(p, "-%lu", journal->j_inode->i_ino);
jbd_debug(1,
"journal %p: inode %s/%ld, size %Ld, bits %d, blksize %ld\n",
--
2.1.3
This makes a very large function a little smaller.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: no changes.
fs/ext4/super.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ca9d4a2fed41..5f3c43a66937 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3420,7 +3420,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
unsigned long journal_devnum = 0;
unsigned long def_mount_opts;
struct inode *root;
- char *cp;
const char *descr;
int ret = -ENOMEM;
int blocksize, clustersize;
@@ -3456,8 +3455,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
#endif
/* Cleanup superblock name */
- for (cp = sb->s_id; (cp = strchr(cp, '/'));)
- *cp = '!';
+ strreplace(sb->s_id, '/', '!');
/* -EINVAL is default */
ret = -EINVAL;
--
2.1.3
On Tue, 2015-06-09 at 01:26 +0200, Rasmus Villemoes wrote:
> Strings are sometimes sanitized by replacing a certain character
> (often '/') by another (often '!').
[]
> v2: spello fixed, parameters renamed 'old' and 'new' (just so the
> kernel doc aligns nicely, and because that's what python -c
> 'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
> caused more bloat), still called strreplace.
OK, thanks. I think the chars should be ints though
just for consistency for strchr variants.
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> @@ -111,6 +111,7 @@ extern int memcmp(const void *,const void *,__kernel_size_t);
> extern void * memchr(const void *,int,__kernel_size_t);
> #endif
> void *memchr_inv(const void *s, int c, size_t n);
> +char *strreplace(char *s, char old, char new);
On Tue, Jun 09, 2015 at 01:26:48AM +0200, Rasmus Villemoes wrote:
> Doing single-character substitution on an entire string is open-coded
> in a few places, sometimes in a rather suboptimal way. This introduces
> a trivial helper, strreplace, for this task along with a few example
> conversions.
>
> Andrew, can I get you to take 1/8 through the mm tree? I'm not sure
> what the easiest path is for the remaining patches.
This is not super urgent, right? So we could let 1/8 go into
mainline, and then the rest of the patches could go in the next
release. That would be the simplest, although it would drag out how
long it would take for strreplace to be used everywhere.
- Ted
On 06/08/2015 05:26 PM, Rasmus Villemoes wrote:
> Part of the disassembly of do_blk_trace_setup:
>
> 231b: e8 00 00 00 00 callq 2320 <do_blk_trace_setup+0x50>
> 231c: R_X86_64_PC32 strlen+0xfffffffffffffffc
> 2320: eb 0a jmp 232c <do_blk_trace_setup+0x5c>
> 2322: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
> 2328: 48 83 c3 01 add $0x1,%rbx
> 232c: 48 39 d8 cmp %rbx,%rax
> 232f: 76 47 jbe 2378 <do_blk_trace_setup+0xa8>
> 2331: 41 80 3c 1c 2f cmpb $0x2f,(%r12,%rbx,1)
> 2336: 75 f0 jne 2328 <do_blk_trace_setup+0x58>
> 2338: 41 c6 04 1c 5f movb $0x5f,(%r12,%rbx,1)
> 233d: 4c 89 e7 mov %r12,%rdi
> 2340: e8 00 00 00 00 callq 2345 <do_blk_trace_setup+0x75>
> 2341: R_X86_64_PC32 strlen+0xfffffffffffffffc
> 2345: eb e1 jmp 2328 <do_blk_trace_setup+0x58>
>
> Yep, that's right: gcc isn't smart enough to realize that replacing
> '/' by '_' cannot change the strlen(), so we call it again and again
> (at least when a '/' is found). Even if gcc were that smart, this
> construction would still loop over the string twice, once for the
> initial strlen() call and then the open-coded loop.
>
> Let's simply use strreplace() instead.
Patch looks fine to me, but there's no strreplace in in Linus' tree.
Dependencies like that should be noted in the patch. Please send
followup patches like this when the main patch is in Linus tree, and I'd
be happy to apply it.
--
Jens Axboe
On Tue, Jun 09 2015, Joe Perches <[email protected]> wrote:
> On Tue, 2015-06-09 at 01:26 +0200, Rasmus Villemoes wrote:
>> Strings are sometimes sanitized by replacing a certain character
>> (often '/') by another (often '!').
> []
>> v2: spello fixed, parameters renamed 'old' and 'new' (just so the
>> kernel doc aligns nicely, and because that's what python -c
>> 'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
>> caused more bloat), still called strreplace.
>
> OK, thanks. I think the chars should be ints though
> just for consistency for strchr variants.
I disagree. That way lies subtle (semi)bugs. Quick quiz: What does
memscan return below?
char a[1] = {X}; /* for some suitable X */
char *p = memscan(a, a[0], 1);
Here's the kerneldoc+prototype for memscan:
/**
* memscan(void *addr, int c, size_t size) - Find a character in an area of memory.
* @addr: The memory area
* @c: The byte to search for
* @size: The size of the area.
*
* returns the address of the first occurrence of @c, or 1 byte past
* the area if @c is not found
*/
So obviously p==a, right? Wrong. Or rather, wrong when char is signed
and X lies outside the ascii range. Or maybe right, if you're on an
architecture with its own memscan that DTRT. And 'the right thing' is
obviously to use only the LSB of c, which would have been harder to get
wrong if c was just a u8 to begin with.
(The only in-tree callers which do not pass an explicit non-negative
constant seem to be in drivers/hid/usbhid/usbkbd.c and
net/bluetooth/hidp/core.c, and they both pass something from an unsigned
char array).
We're stuck with int in the libc functions, but we can do better for new
interfaces.
Rasmus
On Tue, Jun 09 2015, Theodore Ts'o <[email protected]> wrote:
> On Tue, Jun 09, 2015 at 01:26:48AM +0200, Rasmus Villemoes wrote:
>> Doing single-character substitution on an entire string is open-coded
>> in a few places, sometimes in a rather suboptimal way. This introduces
>> a trivial helper, strreplace, for this task along with a few example
>> conversions.
>>
>> Andrew, can I get you to take 1/8 through the mm tree? I'm not sure
>> what the easiest path is for the remaining patches.
>
> This is not super urgent, right?
Right, not urgent at all.
> So we could let 1/8 go into mainline, and then the rest of the patches
> could go in the next release. That would be the simplest, although it
> would drag out how long it would take for strreplace to be used
> everywhere.
Sure, though it would be a little weird to have a helper with no users
until 4.3 comes out.
Rasmus
On Tue, 9 Jun 2015 01:26:48 +0200 Rasmus Villemoes <[email protected]> wrote:
> Doing single-character substitution on an entire string is open-coded
> in a few places, sometimes in a rather suboptimal way. This introduces
> a trivial helper, strreplace, for this task along with a few example
> conversions.
>
> Andrew, can I get you to take 1/8 through the mm tree? I'm not sure
> what the easiest path is for the remaining patches.
With this sort of thing I grab everything them feed the dependent
patches to maintainers after the base patch is upstream.
Or I merge the dependent patches myself if they were acked.
Or if the dependent patches are simple I'll just merge them anyway,
shrug. I'd say these fall into that category.
On Tue, 9 Jun 2015 01:26:54 +0200
Rasmus Villemoes <[email protected]> wrote:
> There's no point in starting over when we meet a '/'. This also
> eliminates a stack variable and a little .text.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> v2: no changes.
>
> drivers/md/md.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 27506302eb7a..2ea2f28551c5 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2024,7 +2024,6 @@ static int bind_rdev_to_array(struct md_rdev
> *rdev, struct mddev *mddev) {
> char b[BDEVNAME_SIZE];
> struct kobject *ko;
> - char *s;
> int err;
>
> /* prevent duplicates */
> @@ -2070,8 +2069,7 @@ static int bind_rdev_to_array(struct md_rdev
> *rdev, struct mddev *mddev) return -EBUSY;
> }
> bdevname(rdev->bdev,b);
> - while ( (s=strchr(b, '/')) != NULL)
> - *s = '!';
> + strreplace(b, '/', '!');
>
> rdev->mddev = mddev;
> printk(KERN_INFO "md: bind<%s>\n", b);
Acked-by: NeilBrown <[email protected]>
I'm happy for Andrew to merge this.
Thanks,
NeilBrown