| Message ID | 20170506052243.GB291@x4 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2017-05-06 at 07:22 +0200, markus@trippelsdorf.de wrote: > - ret = __elevator_change(q, name); > + strlcpy(elevator_name, skip_spaces(name), sizeof(elevator_name)); Hello Markus, 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 Thanks, Bart.
On 2017.05.08 at 15:47 +0000, Bart Van Assche wrote: > On Sat, 2017-05-06 at 07:22 +0200, markus@trippelsdorf.de wrote: > > - 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.
On Mon, 2017-05-08 at 18:48 +0200, markus@trippelsdorf.de wrote: > On 2017.05.08 at 15:47 +0000, Bart Van Assche wrote: > > On Sat, 2017-05-06 at 07:22 +0200, markus@trippelsdorf.de wrote: > > > - 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. Hello Markus, Sorry - I misread your patch. Since that was the only comment I had: Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>
On 05/05/2017 11:22 PM, markus@trippelsdorf.de wrote: > > 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. Added, thanks.
> -----Original Message----- > From: linux-block-owner@vger.kernel.org [mailto:linux-block- > owner@vger.kernel.org] On Behalf Of markus@trippelsdorf.de > Sent: Saturday, May 06, 2017 12:23 AM > To: Bart Van Assche <Bart.VanAssche@sandisk.com> > Cc: linux-block@vger.kernel.org; axboe@kernel.dk > Subject: [PATCH] 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 ... > @@ -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; > } ... > @@ -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) > { ... > + 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; > } That leaves two lines of error prints for a single error, and the second line doesn't convey additional information.
> On May 8, 2017, at 8:18 PM, Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote: > > > >> -----Original Message----- >> From: linux-block-owner@vger.kernel.org [mailto:linux-block- >> owner@vger.kernel.org] On Behalf Of markus@trippelsdorf.de >> Sent: Saturday, May 06, 2017 12:23 AM >> To: Bart Van Assche <Bart.VanAssche@sandisk.com> >> Cc: linux-block@vger.kernel.org; axboe@kernel.dk >> Subject: [PATCH] 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 > ... >> @@ -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; >> } > ... >> @@ -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) >> { > ... >> + 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; >> } > > 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.
On 2017.05.08 at 20:22 -0600, Jens Axboe wrote: > > > On May 8, 2017, at 8:18 PM, Elliott, Robert (Persistent Memory) > > <elliott@hpe.com> wrote: > >> + printk(KERN_ERR "elevator: type %s not found\n", name); > > ... > >> + 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?
On 05/08/2017 10:42 PM, markus@trippelsdorf.de wrote: > On 2017.05.08 at 20:22 -0600, Jens Axboe wrote: >> >>> On May 8, 2017, at 8:18 PM, Elliott, Robert (Persistent Memory) >>> <elliott@hpe.com> wrote: >>>> + printk(KERN_ERR "elevator: type %s not found\n", name); >>> ... >>>> + 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? 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.
diff --git a/block/elevator.c b/block/elevator.c index bf11e70f008b..3e1a42192519 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -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; }
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 <markus@trippelsdorf.de>