block: Remove leading whitespace and trailing newline in elevator switch error message

PATCH In review [email protected] 4 participants 9 messages May 6, 2017 – May 9, 2017 Block layer

lore

Share & tools

lore

[email protected]

author

2017-05-06 05:22 UTC #0 lore raw

PATCH 1/1 block: Remove leading whitespace and 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 leading whitespace and unintended line break.)

Fix by removing the leading whitespace and stripping the trailing newline.

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

block/elevator.c +8 -7

index bf11e70f008b..3e1a42192519 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, skip_spaces(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

Please include the version number of a patch in the e-mail subject line whe= n
posting a second or later version. Regarding the strlcpy() call, in the FreeBSD strlcpy() man page I found the following: "If the src and dst strin= gs
overlap, the behavior is undefined." Since the Linux kernel implementation = of
strlcpy() uses memcpy() and since the ANSI C standard does not allow that memcpy() input and output buffers overlap, please rework this code. See e.g= .
http://pubs.opengroup.org/onlinepubs/9699919799/functions/memcpy.html

Thanks,

Bart.=

[email protected]

author

2017-05-08 16:48 UTC #2 lore raw

Quoted 11 lines · Bart Van Assche

block/elevator.c static inline bool elv_support_iosched(struct request_queue *q) · line 1119
Quoted 2 lines · On Sat, 2017-05-06 at 07:22 +0200, [email protected] wrote:
block/elevator.c +1 -1
-	ret = __elevator_change(q, name);
+	strlcpy(elevator_name, skip_spaces(name), sizeof(elevator_name));
Please include the version number of a patch in the e-mail subject line when posting a second or later version. Regarding the strlcpy() call, in the FreeBSD strlcpy() man page I found the following: "If the src and dst strings overlap, the behavior is undefined." Since the Linux kernel implementation of strlcpy() uses memcpy() and since the ANSI C standard does not allow that memcpy() input and output buffers overlap, please rework this code. See e.g.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/memcpy.html

I don't see how src and dst could possibly overlap here.

Signature · 1 line

Markus

Bart Van Assche

Reviewed-by

2017-05-08 16:52 UTC #3 lore raw

Quoted 6 lines · On Mon, 2017-05-08 at 18:48 +0200, [email protected] wrote:

Quoted 5 lines · Bart Van Assche
Review
Quoted 2 lines · On Sat, 2017-05-06 at 07:22 +0200, [email protected] wrote:
(quoted diff) +1 -1
-	ret =3D __elevator_change(q, name);
+	strlcpy(elevator_name, skip_spaces(name), sizeof(elevator_name));
=20
Please include the version number of a patch in the e-mail subject line=

when

Quoted 2 lines

Quoted 2 lines
posting a second or later version. Regarding the strlcpy() call, in the FreeBSD strlcpy() man page I found the following: "If the src and dst s=

trings

Quoted 1 line

Quoted 1 line
overlap, the behavior is undefined." Since the Linux kernel implementat=

ion of

Quoted 1 line

Quoted 1 line
strlcpy() uses memcpy() and since the ANSI C standard does not allow th=

Quoted 1 line

Quoted 1 line
memcpy() input and output buffers overlap, please rework this code. See=

e.g.

Quoted 3 lines

Quoted 1 line
http://pubs.opengroup.org/onlinepubs/9699919799/functions/memcpy.html
=20
I don't see how src and dst could possibly overlap here.

Hello Markus,

Sorry - I misread your patch. Since that was the only comment I had:

Reviewed-by Bart Van Assche <[email protected]>=

Jens Axboe

maintainer

2017-05-08 16:53 UTC #4 lore raw

In reply to [email protected]

Quoted 6 lines · On 05/05/2017 11:22 PM, [email protected] wrote:

Trying to switch to a non-existing elevator currently results in garbled dmesg output, e.g.:
 # echo "  foo" > /sys/block/sda/queue/scheduler 

Quoted 7 lines

elevator: type foo not found
elevator: switch to foo
failed
(note the leading whitespace and unintended line break.)
Fix by removing the leading whitespace and stripping the trailing newline.

Added, thanks.

Signature · 1 line

Jens Axboe

Elliott, Robert (Persistent Memory)

2017-05-09 02:18 UTC #5 lore raw

In reply to [email protected]

Quoted 21 lines

-----Original Message-----
From: [email protected] [mailto:linux-block- [email protected]] On Behalf Of [email protected]
Sent: Saturday, May 06, 2017 12:23 AM
To: Bart Van Assche <[email protected]>
Cc: [email protected]; [email protected]
Subject: [PATCH] block: Remove leading whitespace and trailing newline in elevator switch error message
=20
=20
Trying to switch to a non-existing elevator currently results in garbled
dmesg output, e.g.:
=20
 # echo "  foo" > /sys/block/sda/queue/scheduler
=20
elevator: type   foo not found
elevator: switch to   foo
 failed

...

Quoted 14 lines

block/elevator.c +0 -0

@@ -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);

=20

(quoted diff) +1 -3

-	strlcpy(elevator_name, name, sizeof(elevator_name));
-	e =3D elevator_get(strstrip(elevator_name), true);
+	e =3D elevator_get(name, true);
 	if (!e) {
-		printk(KERN_ERR "elevator: type %s not found\n",

elevator_name);

block/elevator.c +1 -0

+		printk(KERN_ERR "elevator: type %s not found\n", name);
 		return -EINVAL;
 	}

...

Quoted 5 lines

block/elevator.c +0 -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)
 {

...

Quoted 9 lines

(quoted diff) +1 -0

+	strlcpy(elevator_name, skip_spaces(name),

sizeof(elevator_name));

block/elevator.c +2 -0

+	strstrip(elevator_name);
+	ret =3D __elevator_change(q, elevator_name);
 	if (!ret)
 		return count;

=20

(quoted diff) +1 -1

-	printk(KERN_ERR "elevator: switch to %s failed\n", name);
+	printk(KERN_ERR "elevator: switch to %s failed\n",

Quoted 3 lines

elevator_name);
return ret;
}

That leaves two lines of error prints for a single error, and the second line doesn't convey additional information.

Jens Axboe

maintainer

2017-05-09 02:22 UTC #6 lore raw

Quoted 2 lines

On May 8, 2017, at 8:18 PM, Elliott, Robert (Persistent Memory) <elliott@h=

Quoted 56 lines · pe.com> wrote:

=20
=20
=20
Quoted 19 lines
-----Original Message-----
From: [email protected] [mailto:linux-block- [email protected]] On Behalf Of [email protected]
Sent: Saturday, May 06, 2017 12:23 AM
To: Bart Van Assche <[email protected]>
Cc: [email protected]; [email protected]
Subject: [PATCH] block: Remove leading whitespace and trailing newline in elevator switch error message
=20
=20
Trying to switch to a non-existing elevator currently results in garbled
dmesg output, e.g.:
=20
# echo "  foo" > /sys/block/sda/queue/scheduler
=20
elevator: type   foo not found
elevator: switch to   foo
failed
...
Quoted 14 lines
block/elevator.c +0 -0
@@ -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);
=20
(quoted diff) +1 -3
-    strlcpy(elevator_name, name, sizeof(elevator_name));
-    e =3D elevator_get(strstrip(elevator_name), true);
+    e =3D 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;
}
...
Quoted 5 lines
block/elevator.c +0 -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)
 {
...
Quoted 9 lines
+ strlcpy(elevator_name, skip_spaces(name),
sizeof(elevator_name));
block/elevator.c +2 -0
+    strstrip(elevator_name);
+    ret =3D __elevator_change(q, elevator_name);
   if (!ret)
       return count;
=20
(quoted diff) +1 -1
-    printk(KERN_ERR "elevator: switch to %s failed\n", name);
+    printk(KERN_ERR "elevator: switch to %s failed\n",
Quoted 3 lines
elevator_name);
   return ret;
}

Quoted 3 lines

=20
That leaves two lines of error prints for a single error, and the second line doesn't convey additional information.

Yeah good point. Honestly I think we should just kill both lines. None of th= em add value.=20

[email protected]

author

2017-05-09 04:42 UTC #7 lore raw

Quoted 4 lines · Jens Axboe

Quoted 4 lines
Quoted 1 line · Robert (Persistent Memory)
+ printk(KERN_ERR "elevator: type %s not found\n", name);

Quoted 8 lines

Quoted 5 lines
...
Quoted 1 line
+ printk(KERN_ERR "elevator: switch to %s failed\n",
That leaves two lines of error prints for a single error, and the second line doesn't convey additional information.
Yeah good point. Honestly I think we should just kill both lines. None of them add value.

They at least help debugging of udev rules, e.g. on Gentoo they have:

 # set noop on solid state drives
 SUBSYSTEM=="block", ACTION=="add", ATTR{queue/rotational}=="0", ATTR{queue/scheduler}="noop"

And this will of course fail for mq. But how will you notice it without any dmesg error message?

Signature · 1 line

Markus

Jens Axboe

maintainer

2017-05-09 14:57 UTC #8 lore raw

Quoted 14 lines · On 05/08/2017 10:42 PM, [email protected] wrote:

Quoted 4 lines · Jens Axboe
Quoted 4 lines
Quoted 1 line · Robert (Persistent Memory)
+ printk(KERN_ERR "elevator: type %s not found\n", name);
Quoted 8 lines
Quoted 5 lines
...
Quoted 1 line
+ printk(KERN_ERR "elevator: switch to %s failed\n",
That leaves two lines of error prints for a single error, and the second line doesn't convey additional information.
Yeah good point. Honestly I think we should just kill both lines. None of them add value.

Quoted 7 lines

They at least help debugging of udev rules, e.g. on Gentoo they have:
 # set noop on solid state drives
 SUBSYSTEM=="block", ACTION=="add", ATTR{queue/rotational}=="0", ATTR{queue/scheduler}="noop"
And this will of course fail for mq. But how will you notice it without any dmesg error message?

The echo to the sysfs file fails. udev should report such failures. Using dmesg as the channel to report and discover errors like that is not a great idea, and it was a mistake to put that code in the switch path in the first place.

Signature · 1 line

Jens Axboe