lib/string_helpers et al.: Change return value of strreplace()

PATCH Superseded Andy Shevchenko 4 participants 12 messages Mar 23, 2023 – Jun 5, 2023

Subsystems (4)

Library code Driver core Journalling layer for block devices (JBD2) Generic string library

lore

Share & tools

lore

Andy Shevchenko

author

2023-03-23 12:36 UTC #0 lore raw

v2 cover lib/string_helpers et al.: Change return value of strreplace()

It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.

The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.

In v2:
- removed not anymore used variable (LKP)
- added tag (Jan)
- fixed wording (Kees)
- actually return the pointer to the string itself

Andy Shevchenko (3):
jbd2: Avoid printing outside the boundary of the buffer lib/string_helpers: Change returned value of the strreplace()
kobject: Use return value of strreplace()

 fs/jbd2/journal.c      |  6 ++----
 include/linux/string.h |  2 +-
 lib/kobject.c          |  3 +--
 lib/string_helpers.c   | 12 ++++++++----
 4 files changed, 12 insertions(+), 11 deletions(-)

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-03-23 12:36 UTC #1 lore raw

PATCH v2 1/3 jbd2: Avoid printing outside the boundary of the buffer

Theoretically possible that "%pg" will take all room for the j_devname and hence the "-%lu" will go outside the boundary due to unconditional sprintf() in use. To make this code more robust, replace two sequential s*printf():s by a single call and then replace forbidden character. It's possible to do this way, because '/' won't ever be in the result of "-%lu".

Signed-off-by Andy Shevchenko <[email protected]>

Reviewed-by Jan Kara <[email protected]>

---
 fs/jbd2/journal.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

fs/jbd2/journal.c +2 -4

index 8ae419152ff6..6e17f8f94dfd 100644
@@ -1491,7 +1491,6 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 {
 	journal_t *journal;
 	sector_t blocknr;
-	char *p;
 	int err = 0;
 
 	blocknr = 0;

@@ -1515,9 +1514,8 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 
 	journal->j_inode = inode;
 	snprintf(journal->j_devname, sizeof(journal->j_devname),
-		 "%pg", journal->j_dev);
-	p = strreplace(journal->j_devname, '/', '!');
-	sprintf(p, "-%lu", journal->j_inode->i_ino);
+		 "%pg-%lu", journal->j_dev, journal->j_inode->i_ino);
+	strreplace(journal->j_devname, '/', '!');
 	jbd2_stats_proc_init(journal);
 
 	return journal;

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-03-23 12:36 UTC #2 lore raw

PATCH v2 2/3 lib/string_helpers: Change returned value of the strreplace()

In reply to Andy Shevchenko

It's more useful to return the pointer to the string itself with strreplace(), so it may be used like

	attr->name = strreplace(name, '/', '_');

While at it, amend the kernel documentation.

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/string.h |  2 +-
 lib/string_helpers.c   | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

include/linux/string.h +1 -1

index a7bff7ed3cb0..cb0c24ce0826 100644
@@ -169,7 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
 #endif
 
 void *memchr_inv(const void *s, int c, size_t n);
-char *strreplace(char *s, char old, char new);
+char *strreplace(char *str, char old, char new);
 
 extern void kfree_const(const void *x);

lib/string_helpers.c +8 -4

index 230020a2e076..d3b1dd718daf 100644
@@ -979,18 +979,22 @@ EXPORT_SYMBOL(__sysfs_match_string);
 
 /**
  * strreplace - Replace all occurrences of character in string.
- * @s: The string to operate on.
+ * @str: 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.
+ * Replaces the each @old character with a @new one in the given string @str.
+ *
+ * Return: pointer to the string @str itself.
  */
-char *strreplace(char *s, char old, char new)
+char *strreplace(char *str, char old, char new)
 {
+	char *s = str;
+
 	for (; *s; ++s)
 		if (*s == old)
 			*s = new;
-	return s;
+	return str;
 }
 EXPORT_SYMBOL(strreplace);

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-03-23 12:36 UTC #3 lore raw

PATCH v2 3/3 kobject: Use return value of strreplace()

In reply to Andy Shevchenko

Since strreplace() returns the pointer to the string itself, we may use it directly in the code.

Signed-off-by Andy Shevchenko <[email protected]>

---
 lib/kobject.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

lib/kobject.c +1 -2

index f79a434e1231..16d530f9c174 100644
@@ -281,8 +281,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 		kfree_const(s);
 		if (!t)
 			return -ENOMEM;
-		strreplace(t, '/', '!');
-		s = t;
+		s = strreplace(t, '/', '!');
 	}
 	kfree_const(kobj->name);
 	kobj->name = s;

Signature · 1 line

2.40.0.1.gaa8946217a0b

David Laight

2023-03-26 13:37 UTC #4 lore raw

RE: [PATCH v2 3/3] kobject: Use return value of strreplace()

From: Andy Shevchenko

Quoted 15 lines

Sent: 23 March 2023 12:37
Since strreplace() returns the pointer to the string itself, we may use it directly in the code.
Signed-off-by Andy Shevchenko <[email protected]>
---
 lib/kobject.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
lib/kobject.c +0 -0
index f79a434e1231..16d530f9c174 100644

lib/kobject.c int kobject_set_name_vargs(struct kobject *kobj, const char *fmt, · line 284

Quoted 6 lines

lib/kobject.c +1 -2

@@ -281,8 +281,7 @@ int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 		kfree_const(s);
 		if (!t)
 			return -ENOMEM;
-		strreplace(t, '/', '!');
-		s = t;
+		s = strreplace(t, '/', '!');

Why do this? It just makes the code harder to read because you have to know another 'silly fact' about a function.

Possibly useful return values might be:
1) The address of the first changed character.
2) The address of the last changed characher.
3) The '\0' terminator.
4) void.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)

Andy Shevchenko

author

2023-04-05 13:34 UTC #5 lore raw

In reply to Andy Shevchenko

Quoted 7 lines · Andy Shevchenko

It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.

Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?

Quoted 16 lines

In v2:
- removed not anymore used variable (LKP)
- added tag (Jan)
- fixed wording (Kees)
- actually return the pointer to the string itself
Andy Shevchenko (3):
jbd2: Avoid printing outside the boundary of the buffer lib/string_helpers: Change returned value of the strreplace()
kobject: Use return value of strreplace()
 fs/jbd2/journal.c      |  6 ++----
 include/linux/string.h |  2 +-
 lib/kobject.c          |  3 +--
 lib/string_helpers.c   | 12 ++++++++----
 4 files changed, 12 insertions(+), 11 deletions(-)

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2023-04-05 14:25 UTC #6 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?

Sure, want me to take them all through my driver-core tree?

thanks,

greg k-h

Andy Shevchenko

author

2023-04-05 14:39 UTC #7 lore raw

Quoted 10 lines · Greg Kroah-Hartman

Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.

Quoted 4 lines

Quoted 2 lines
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?
Sure, want me to take them all through my driver-core tree?

Fine by me! Dunno about others. Kees?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Kees Cook

maintainer

2023-04-06 02:58 UTC #8 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Greg Kroah-Hartman
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?
Sure, want me to take them all through my driver-core tree?
Fine by me! Dunno about others. Kees?

Yeah, that's cool by me. :)

Signature · 1 line

Kees Cook

Andy Shevchenko

author

2023-06-05 14:06 UTC #9 lore raw

Quoted 12 lines · Kees Cook

Quoted 11 lines · Andy Shevchenko
Quoted 10 lines · Greg Kroah-Hartman
Quoted 9 lines · Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?
Sure, want me to take them all through my driver-core tree?
Fine by me! Dunno about others. Kees?
Yeah, that's cool by me. :)

Greg, does this slip through the cracks?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Greg Kroah-Hartman

maintainer

2023-06-05 16:57 UTC #10 lore raw

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Kees Cook
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Greg Kroah-Hartman
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Sure, want me to take them all through my driver-core tree?
Fine by me! Dunno about others. Kees?
Yeah, that's cool by me. :)
Greg, does this slip through the cracks?

It did. Can someone resend this?

thanks,

greg k-h

Andy Shevchenko

author

2023-06-05 17:07 UTC #11 lore raw

Quoted 19 lines · Greg Kroah-Hartman

Quoted 18 lines · Andy Shevchenko
Quoted 17 lines · Kees Cook
Quoted 16 lines · Andy Shevchenko
Quoted 14 lines · Greg Kroah-Hartman
Quoted 11 lines · Andy Shevchenko
Quoted 7 lines · Andy Shevchenko
It's more convenient to have strreplace() to return the pointer to
the string itself. This will help users to make their code better.
The patch 1 kills the only user of the returned value of strreplace(), Patch 2 converts the return value of strreplace(). And patch 3 shows how it may be useful. That said, the series can be routed via fs tree, with or without the last patch.
Since there are no comments, who can apply this (patches 1 and 2)? Greg, are you fine with the kobject change?
Sure, want me to take them all through my driver-core tree?

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Fine by me! Dunno about others. Kees?
Yeah, that's cool by me. :)
Greg, does this slip through the cracks?
It did. Can someone resend this?

Done as v3. [email protected]

Signature · 2 lines

With Best Regards,
Andy Shevchenko