* [RFC/PATCH 0/8] Introduce strreplace
@ 2015-06-04 9:37 Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 1/8] lib: string: " Rasmus Villemoes
` (7 more replies)
0 siblings, 8 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Greg Kroah-Hartman, Neil Brown, Theodore Ts'o,
Andreas Dilger, Steven Rostedt, Ingo Molnar
Cc: Rasmus Villemoes, linux-kernel, linux-raid, linux-ext4
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.
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 | 5 +----
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 | 4 +---
lib/string.c | 17 +++++++++++++++++
9 files changed, 28 insertions(+), 28 deletions(-)
--
2.1.3
^ permalink raw reply [flat|nested] 18+ messages in thread
* [RFC/PATCH 1/8] lib: string: Introduce strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-04 10:56 ` Joe Perches
2015-06-04 9:37 ` [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace Rasmus Villemoes
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Daniel Borkmann, Herbert Xu; +Cc: Rasmus Villemoes, linux-kernel
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 <linux@rasmusvillemoes.dk>
---
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..4f09e41c7169 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);
+extern char *strreplace(char *s, char bad, char good);
extern void kfree_const(const void *x);
diff --git a/lib/string.c b/lib/string.c
index bb3d4b6993c4..ef52e86f6b87 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 occurences of character in string
+ * @s: The string to operate on
+ * @bad: The character being replaced
+ * @good: The character @bad is replaced with.
+ *
+ * Returns pointer to the nul byte at the end of @s.
+ */
+char *strreplace(char *s, char bad, char good)
+{
+ for (; *s; ++s)
+ if (*s == bad)
+ *s = good;
+ return s;
+}
+EXPORT_SYMBOL(strreplace);
--
2.1.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 1/8] lib: string: " Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-08 15:49 ` Steven Rostedt
2015-06-04 9:37 ` [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup Rasmus Villemoes
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Steven Rostedt, Ingo Molnar; +Cc: Rasmus Villemoes, linux-kernel
There's no point in starting over every time we see a ','...
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 1/8] lib: string: " Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-08 16:03 ` Steven Rostedt
2015-06-04 9:37 ` [RFC/PATCH 4/8] lib/kobject.c: Use strreplace Rasmus Villemoes
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Steven Rostedt, Ingo Molnar; +Cc: Rasmus Villemoes, linux-kernel
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 <linux@rasmusvillemoes.dk>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 4/8] lib/kobject.c: Use strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
` (2 preceding siblings ...)
2015-06-04 9:37 ` [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:31 ` Al Viro
2015-06-04 9:37 ` [RFC/PATCH 5/8] drivers/base/core.c: " Rasmus Villemoes
` (3 subsequent siblings)
7 siblings, 2 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Greg Kroah-Hartman; +Cc: Rasmus Villemoes, linux-kernel
There's probably not many slashes in kobj->name, but starting over
when we see one feels wrong.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
lib/kobject.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index 3b841b97fccd..597d962d3d4d 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -258,7 +258,6 @@ 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;
@@ -270,8 +269,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
}
/* ewww... some of these buggers have '/' in the name ... */
- while ((s = strchr(kobj->name, '/')))
- s[0] = '!';
+ strreplace((char *)kobj->name, '/', '!');
kfree(old_name);
return 0;
--
2.1.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 5/8] drivers/base/core.c: Use strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
` (3 preceding siblings ...)
2015-06-04 9:37 ` [RFC/PATCH 4/8] lib/kobject.c: Use strreplace Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:28 ` Al Viro
2015-06-04 9:37 ` [RFC/PATCH 6/8] drivers/md/md.c: " Rasmus Villemoes
` (2 subsequent siblings)
7 siblings, 2 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Greg Kroah-Hartman; +Cc: Rasmus Villemoes, linux-kernel
This eliminates a local variable and a little .text.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
drivers/base/core.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 21d13038534e..01f2c1214f06 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1282,8 +1282,6 @@ const char *device_get_devnode(struct device *dev,
umode_t *mode, kuid_t *uid, kgid_t *gid,
const char **tmp)
{
- char *s;
-
*tmp = NULL;
/* the device type may provide a specific name */
@@ -1306,8 +1304,7 @@ const char *device_get_devnode(struct device *dev,
*tmp = kstrdup(dev_name(dev), GFP_KERNEL);
if (!*tmp)
return NULL;
- while ((s = strchr(*tmp, '!')))
- s[0] = '/';
+ strreplace((char *)*tmp, '!', '/');
return *tmp;
}
--
2.1.3
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 6/8] drivers/md/md.c: Use strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
` (4 preceding siblings ...)
2015-06-04 9:37 ` [RFC/PATCH 5/8] drivers/base/core.c: " Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 7/8] fs/jbd2/journal.c: " Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 8/8] fs/ext4/super.c: Use strreplace in ext4_fill_super Rasmus Villemoes
7 siblings, 0 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Neil Brown; +Cc: Rasmus Villemoes, linux-raid, linux-kernel
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 <linux@rasmusvillemoes.dk>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 7/8] fs/jbd2/journal.c: Use strreplace
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
` (5 preceding siblings ...)
2015-06-04 9:37 ` [RFC/PATCH 6/8] drivers/md/md.c: " Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 8/8] fs/ext4/super.c: Use strreplace in ext4_fill_super Rasmus Villemoes
7 siblings, 0 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Theodore Ts'o
Cc: Rasmus Villemoes, linux-ext4, linux-kernel
In one case, we eliminate a local variable; in the other a strlen()
call and some .text.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [RFC/PATCH 8/8] fs/ext4/super.c: Use strreplace in ext4_fill_super
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
` (6 preceding siblings ...)
2015-06-04 9:37 ` [RFC/PATCH 7/8] fs/jbd2/journal.c: " Rasmus Villemoes
@ 2015-06-04 9:37 ` Rasmus Villemoes
7 siblings, 0 replies; 18+ messages in thread
From: Rasmus Villemoes @ 2015-06-04 9:37 UTC (permalink / raw)
To: Andrew Morton, Theodore Ts'o, Andreas Dilger
Cc: Rasmus Villemoes, linux-ext4, linux-kernel
This makes a very large function a little smaller.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 1/8] lib: string: Introduce strreplace
2015-06-04 9:37 ` [RFC/PATCH 1/8] lib: string: " Rasmus Villemoes
@ 2015-06-04 10:56 ` Joe Perches
2015-06-04 22:37 ` Al Viro
0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2015-06-04 10:56 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Daniel Borkmann, Herbert Xu, linux-kernel
On Thu, 2015-06-04 at 11:37 +0200, Rasmus Villemoes wrote:
> 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).
You used in one of the follow-on patches too.
> diff --git 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 occurences of character in string
occurrences
> + * @s: The string to operate on
> + * @bad: The character being replaced
> + * @good: The character @bad is replaced with.
> + *
> + * Returns pointer to the nul byte at the end of @s.
> + */
> +char *strreplace(char *s, char bad, char good)
> +{
> + for (; *s; ++s)
> + if (*s == bad)
> + *s = good;
> + return s;
> +}
> +EXPORT_SYMBOL(strreplace);
Seems sensible, but the name maybe could be be more
explicit as strreplace seems like it should more like
a string substitution rather than a char substitution.
Maybe strsubstchr or something like it (strtranschr?)
Because it's so tiny, perhaps this could be inline
instead of EXPORT_SYMBOL.
Maybe from and to instead of good and bad arguments.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 4/8] lib/kobject.c: Use strreplace
2015-06-04 9:37 ` [RFC/PATCH 4/8] lib/kobject.c: Use strreplace Rasmus Villemoes
@ 2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:31 ` Al Viro
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2015-06-04 21:56 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
On Thu, Jun 04, 2015 at 11:37:11AM +0200, Rasmus Villemoes wrote:
> There's probably not many slashes in kobj->name, but starting over
> when we see one feels wrong.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> lib/kobject.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 5/8] drivers/base/core.c: Use strreplace
2015-06-04 9:37 ` [RFC/PATCH 5/8] drivers/base/core.c: " Rasmus Villemoes
@ 2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:28 ` Al Viro
1 sibling, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2015-06-04 21:56 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel
On Thu, Jun 04, 2015 at 11:37:12AM +0200, Rasmus Villemoes wrote:
> This eliminates a local variable and a little .text.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> drivers/base/core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 5/8] drivers/base/core.c: Use strreplace
2015-06-04 9:37 ` [RFC/PATCH 5/8] drivers/base/core.c: " Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
@ 2015-06-04 22:28 ` Al Viro
1 sibling, 0 replies; 18+ messages in thread
From: Al Viro @ 2015-06-04 22:28 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Greg Kroah-Hartman, linux-kernel
On Thu, Jun 04, 2015 at 11:37:12AM +0200, Rasmus Villemoes wrote:
> This eliminates a local variable and a little .text.
Hmm...
> *tmp = kstrdup(dev_name(dev), GFP_KERNEL);
> if (!*tmp)
> return NULL;
> - while ((s = strchr(*tmp, '!')))
> - s[0] = '/';
> + strreplace((char *)*tmp, '!', '/');
> return *tmp;
That cast isn't nice. Why not make it
s = kstrdup(...)
if (s)
strreplace(s, ...)
return *tmp = s;
instead?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 4/8] lib/kobject.c: Use strreplace
2015-06-04 9:37 ` [RFC/PATCH 4/8] lib/kobject.c: Use strreplace Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
@ 2015-06-04 22:31 ` Al Viro
1 sibling, 0 replies; 18+ messages in thread
From: Al Viro @ 2015-06-04 22:31 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Greg Kroah-Hartman, linux-kernel
On Thu, Jun 04, 2015 at 11:37:11AM +0200, Rasmus Villemoes wrote:
> There's probably not many slashes in kobj->name, but starting over
> when we see one feels wrong.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> lib/kobject.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3b841b97fccd..597d962d3d4d 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -258,7 +258,6 @@ 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;
> @@ -270,8 +269,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
> }
>
> /* ewww... some of these buggers have '/' in the name ... */
> - while ((s = strchr(kobj->name, '/')))
> - s[0] = '!';
> + strreplace((char *)kobj->name, '/', '!');
Again, I'd rather do
s = kvasprintf(GFP_KERNEL, fmt, vargs);
if (!s)
return -ENOMEM;
strreplace(s, '/', '!');
old_name = kobj->name;
kobj->name = s;
kfree(old_name);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 1/8] lib: string: Introduce strreplace
2015-06-04 10:56 ` Joe Perches
@ 2015-06-04 22:37 ` Al Viro
2015-06-04 22:45 ` Joe Perches
0 siblings, 1 reply; 18+ messages in thread
From: Al Viro @ 2015-06-04 22:37 UTC (permalink / raw)
To: Joe Perches
Cc: Rasmus Villemoes, Andrew Morton, Daniel Borkmann, Herbert Xu,
linux-kernel
On Thu, Jun 04, 2015 at 03:56:07AM -0700, Joe Perches wrote:
> Seems sensible, but the name maybe could be be more
> explicit as strreplace seems like it should more like
> a string substitution rather than a char substitution.
>
> Maybe strsubstchr or something like it (strtranschr?)
Why not go the full monty and call it strtrtsstrrrtst(), with
strtrtssstrrtst() doing almost, but not quite the same thing?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 1/8] lib: string: Introduce strreplace
2015-06-04 22:37 ` Al Viro
@ 2015-06-04 22:45 ` Joe Perches
0 siblings, 0 replies; 18+ messages in thread
From: Joe Perches @ 2015-06-04 22:45 UTC (permalink / raw)
To: Al Viro
Cc: Rasmus Villemoes, Andrew Morton, Daniel Borkmann, Herbert Xu,
linux-kernel
On Thu, 2015-06-04 at 23:37 +0100, Al Viro wrote:
> On Thu, Jun 04, 2015 at 03:56:07AM -0700, Joe Perches wrote:
> > Seems sensible, but the name maybe could be be more
> > explicit as strreplace seems like it should more like
> > a string substitution rather than a char substitution.
> >
> > Maybe strsubstchr or something like it (strtranschr?)
>
> Why not go the full monty and call it strtrtsstrrrtst(), with
> strtrtssstrrtst() doing almost, but not quite the same thing?
Presumably the extra r is for reverse and the
extra s in for insensitive, just because...
Naming does matter. Consistency too.
And for the consistency bit, the char arguments
should probably be int.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace
2015-06-04 9:37 ` [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace Rasmus Villemoes
@ 2015-06-08 15:49 ` Steven Rostedt
0 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2015-06-08 15:49 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Ingo Molnar, linux-kernel
On Thu, 4 Jun 2015 11:37:09 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> There's no point in starting over every time we see a ','...
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> 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, ',', ' ');
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
>
> re = argv_split(GFP_KERNEL, str, count);
> kfree(str);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup
2015-06-04 9:37 ` [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup Rasmus Villemoes
@ 2015-06-08 16:03 ` Steven Rostedt
0 siblings, 0 replies; 18+ messages in thread
From: Steven Rostedt @ 2015-06-08 16:03 UTC (permalink / raw)
To: Rasmus Villemoes; +Cc: Andrew Morton, Ingo Molnar, linux-kernel, Jens Axboe
blktrace is maintained by Jens Axboe (added to the Cc), although I also
maintain it for cleanups such as this. That's the reason it doesn't
have a "trace_" in its name like the other files do (a bit of trivia for
you ;-)
kernel/trace/blktrace.c should probably be included in the MAINTAINERS
file under BLOCK LAYER.
On Thu, 4 Jun 2015 11:37:10 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> 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.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> 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++)
Wow! I'm surprised this wasn't:
for (i = 0; buts->name[i]; i++)
> - if (buts->name[i] == '/')
> - buts->name[i] = '_';
> + strreplace(buts->name, '/', '_');
>
> bt = kzalloc(sizeof(*bt), GFP_KERNEL);
> if (!bt)
Acked-by: Steven Rostedt <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-06-08 16:03 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-04 9:37 [RFC/PATCH 0/8] Introduce strreplace Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 1/8] lib: string: " Rasmus Villemoes
2015-06-04 10:56 ` Joe Perches
2015-06-04 22:37 ` Al Viro
2015-06-04 22:45 ` Joe Perches
2015-06-04 9:37 ` [RFC/PATCH 2/8] kernel/trace/trace_events_filter.c: Use strreplace Rasmus Villemoes
2015-06-08 15:49 ` Steven Rostedt
2015-06-04 9:37 ` [RFC/PATCH 3/8] blktrace: use strreplace in do_blk_trace_setup Rasmus Villemoes
2015-06-08 16:03 ` Steven Rostedt
2015-06-04 9:37 ` [RFC/PATCH 4/8] lib/kobject.c: Use strreplace Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:31 ` Al Viro
2015-06-04 9:37 ` [RFC/PATCH 5/8] drivers/base/core.c: " Rasmus Villemoes
2015-06-04 21:56 ` Greg Kroah-Hartman
2015-06-04 22:28 ` Al Viro
2015-06-04 9:37 ` [RFC/PATCH 6/8] drivers/md/md.c: " Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 7/8] fs/jbd2/journal.c: " Rasmus Villemoes
2015-06-04 9:37 ` [RFC/PATCH 8/8] fs/ext4/super.c: Use strreplace in ext4_fill_super Rasmus Villemoes
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).