Remove trailing newline in elevator switch error message

PATCH In review Markus Trippelsdorf 2 participants 7 messages Apr 29, 2017 – May 6, 2017 Block layer

lore

Share & tools

lore

Markus Trippelsdorf

author

2017-04-29 05:44 UTC #0 lore raw

PATCH 1/1 Remove trailing newline in elevator switch error message

Trying to switch to a non-existing elevator currently results in garbled dmesg output, e.g.:

 # echo "foo" > /sys/block/sda/queue/scheduler

elevator: type foo not found
elevator: switch to foo
failed

(note the unintended line break.)

Fix by stripping the trailing newline.

block/elevator.c +4 -1

index bf11e70f008b..4f13fcd3c626 100644
@@ -1112,6 +1112,7 @@ static inline bool elv_support_iosched(struct request_queue *q)
 ssize_t elv_iosched_store(struct request_queue *q, const char *name,
 			  size_t count)
 {
+	char elevator_name[ELV_NAME_MAX];
 	int ret;
 
 	if (!(q->mq_ops || q->request_fn) || !elv_support_iosched(q))

@@ -1121,7 +1122,9 @@ ssize_t elv_iosched_store(struct request_queue *q, const char *name,
 	if (!ret)
 		return count;
 
-	printk(KERN_ERR "elevator: switch to %s failed\n", name);
+	strlcpy(elevator_name, name, sizeof(elevator_name));
+	strstrip(elevator_name);
+	printk(KERN_ERR "elevator: switch to %s failed\n", elevator_name);
 	return ret;
 }

Signature · 1 line

Markus

Your patch duplicates the code to remove trailing whitespace which is not very elegant. Please move the code that removes trailing whitespace from __elevator_change() into elv_iosched_store() instead of duplicating it.

Thanks,

Bart.=20

[email protected]

author

2017-05-01 15:50 UTC #2 lore raw

Quoted 39 lines · Bart Van Assche

Quoted 26 lines · Markus Trippelsdorf
Trying to switch to a non-existing elevator currently results in garbled dmesg output, e.g.:
 # echo "foo" > /sys/block/sda/queue/scheduler 
elevator: type foo not found
elevator: switch to foo
failed
(note the unintended line break.)
Fix by stripping the trailing newline.
block/elevator.c +1 -0
index bf11e70f008b..4f13fcd3c626 100644
@@ -1112,6 +1112,7 @@ static inline bool elv_support_iosched(struct request_queue *q)
 ssize_t elv_iosched_store(struct request_queue *q, const char *name,
 			  size_t count)
 {
+	char elevator_name[ELV_NAME_MAX];
 	int ret;
 
 	if (!(q->mq_ops || q->request_fn) || !elv_support_iosched(q))
block/elevator.c ssize_t elv_iosched_store(struct request_queue *q, const char *name, · line 1129
Quoted 9 lines
block/elevator.c +3 -1
@@ -1121,7 +1122,9 @@ ssize_t elv_iosched_store(struct request_queue *q, const char *name,
 	if (!ret)
 		return count;
 
-	printk(KERN_ERR "elevator: switch to %s failed\n", name);
+	strlcpy(elevator_name, name, sizeof(elevator_name));
+	strstrip(elevator_name);
+	printk(KERN_ERR "elevator: switch to %s failed\n", elevator_name);
 	return ret;
 }
Hello Markus,

Quoted 3 lines

Your patch duplicates the code to remove trailing whitespace which is not very elegant. Please move the code that removes trailing whitespace from __elevator_change() into elv_iosched_store() instead of duplicating it.

Yes, I thought about it, but the problem is that elevator_change() calls __elevator_change(), too.

Signature · 1 line

Markus

Bart Van Assche

2017-05-01 15:58 UTC #3 lore raw

Quoted 2 lines · On Mon, 2017-05-01 at 17:49 +0200, [email protected] wrote:

Quoted 1 line · Bart Van Assche
Your patch duplicates the code to remove trailing whitespace which is n=

Quoted 1 line

Quoted 1 line
very elegant. Please move the code that removes trailing whitespace fro=

Quoted 1 line

Quoted 1 line
__elevator_change() into elv_iosched_store() instead of duplicating it=

Quoted 2 lines

=20
Yes, I thought about it, but the problem is that elevator_change() calls=

=20

Quoted 1 line

__elevator_change(), too.

Hello Markus,

In kernel v4.11 elevator_change() only has a single caller and that caller passes an elevator name with no trailing whitespace. I am aware that the elevator_change() function is exported and hence could be used by out-of-tr= ee
kernel drivers. However, we do not care about potential behavior changes fo= r
out-of-tree drivers.

Bart.=

[email protected]

author

2017-05-01 16:29 UTC #4 lore raw

Trying to switch to a non-existing elevator currently results in garbled dmesg output, e.g.:

 # echo "foo" > /sys/block/sda/queue/scheduler

elevator: type foo not found
elevator: switch to foo
failed

(note the unintended line break.)

Fix by stripping the trailing newline.

Signed-off-by Markus Trippelsdorf <[email protected]>

block/elevator.c +8 -7

index bf11e70f008b..1af23eee4395 100644
@@ -1054,7 +1054,6 @@ static int elevator_switch(struct request_queue *q, struct elevator_type *new_e)
  */
 static int __elevator_change(struct request_queue *q, const char *name)
 {
-	char elevator_name[ELV_NAME_MAX];
 	struct elevator_type *e;
 
 	/*

@@ -1063,15 +1062,14 @@ static int __elevator_change(struct request_queue *q, const char *name)
 	if (q->mq_ops && !strncmp(name, "none", 4))
 		return elevator_switch(q, NULL);
 
-	strlcpy(elevator_name, name, sizeof(elevator_name));
-	e = elevator_get(strstrip(elevator_name), true);
+	e = elevator_get(name, true);
 	if (!e) {
-		printk(KERN_ERR "elevator: type %s not found\n", elevator_name);
+		printk(KERN_ERR "elevator: type %s not found\n", name);
 		return -EINVAL;
 	}
 
 	if (q->elevator &&
-	    !strcmp(elevator_name, q->elevator->type->elevator_name)) {
+	    !strcmp(name, q->elevator->type->elevator_name)) {
 		elevator_put(e);
 		return 0;
 	}

@@ -1112,16 +1110,19 @@ static inline bool elv_support_iosched(struct request_queue *q)
 ssize_t elv_iosched_store(struct request_queue *q, const char *name,
 			  size_t count)
 {
+	char elevator_name[ELV_NAME_MAX];
 	int ret;
 
 	if (!(q->mq_ops || q->request_fn) || !elv_support_iosched(q))
 		return count;
 
-	ret = __elevator_change(q, name);
+	strlcpy(elevator_name, name, sizeof(elevator_name));
+	strstrip(elevator_name);
+	ret = __elevator_change(q, elevator_name);
 	if (!ret)
 		return count;
 
-	printk(KERN_ERR "elevator: switch to %s failed\n", name);
+	printk(KERN_ERR "elevator: switch to %s failed\n", elevator_name);
 	return ret;
 }

Signature · 1 line

Markus

Bart Van Assche

2017-05-05 20:05 UTC #5 lore raw

block/elevator.c static inline bool elv_support_iosched(struct request_queue *q)

Quoted 3 lines · On Mon, 2017-05-01 at 18:29 +0200, [email protected] wrote:

block/elevator.c +3 -0

+	strlcpy(elevator_name, name, sizeof(elevator_name));
+	strstrip(elevator_name);
+	ret =3D __elevator_change(q, elevator_name);

Hello Markus,

Are you aware that the current implementation of __elevator_change() strips leading whitespace but that with your patch applied leading whitespace is n= o
longer removed from the elevator name? If you have a look at strim() in lib/string.c you will see that in case of leading whitespace that function does not remove the leading whitespace but returns a pointer to the first non-whitespace character.

Otherwise this patch looks fine to me.

Bart.=

[email protected]

author

2017-05-06 05:15 UTC #6 lore raw

Quoted 12 lines · Bart Van Assche

block/elevator.c static inline bool elv_support_iosched(struct request_queue *q) · line 1121
Quoted 3 lines · On Mon, 2017-05-01 at 18:29 +0200, [email protected] wrote:
block/elevator.c +3 -0
+	strlcpy(elevator_name, name, sizeof(elevator_name));
+	strstrip(elevator_name);
+	ret = __elevator_change(q, elevator_name);
Are you aware that the current implementation of __elevator_change() strips leading whitespace but that with your patch applied leading whitespace is no longer removed from the elevator name? If you have a look at strim() in lib/string.c you will see that in case of leading whitespace that function does not remove the leading whitespace but returns a pointer to the first non-whitespace character.

Well, only in the call to elevator_get(). The dmesg output does contain leading whitespace. But the fix is simple. I will send a new patch.

Signature · 1 line

Markus