* [PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop @ 2019-06-25 11:40 Florian Knauf 2019-06-25 15:42 ` Chaitanya Kulkarni 0 siblings, 1 reply; 6+ messages in thread From: Florian Knauf @ 2019-06-25 11:40 UTC (permalink / raw) To: Jens Axboe Cc: linux-kernel, linux-block, linux-kernel, Florian Knauf, Christian Ewert This patch removes the deprecated simple_strtol function from the option parsing logic in the loopback device driver. It also introduces a range check for the max_loop parameter to ensure that negative and out-of-range values (that cannot be represented by int max_loop) are ignored. Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> --- drivers/block/loop.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 102d79575895..acdd028ed486 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -2289,7 +2289,17 @@ module_exit(loop_exit); #ifndef MODULE static int __init max_loop_setup(char *str) { - max_loop = simple_strtol(str, NULL, 0); + long max_loop_long = 0; + + /* + * Range check for max_loop: negative values and values not + * representable by int are ignored. + */ + if (kstrtol(str, 0, &max_loop_long) == 0 && + max_loop_long >= 0 && + max_loop_long <= INT_MAX) + max_loop = (int) max_loop_long; + return 1; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop 2019-06-25 11:40 [PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop Florian Knauf @ 2019-06-25 15:42 ` Chaitanya Kulkarni 2019-06-25 17:55 ` [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code Florian Knauf 0 siblings, 1 reply; 6+ messages in thread From: Chaitanya Kulkarni @ 2019-06-25 15:42 UTC (permalink / raw) To: Florian Knauf, Jens Axboe Cc: linux-kernel, linux-block, linux-kernel, Christian Ewert Thanks for your patch. On 06/25/2019 04:47 AM, Florian Knauf wrote: > This patch removes the deprecated simple_strtol function from the option > parsing logic in the loopback device driver. It also introduces a range > check for the max_loop parameter to ensure that negative and out-of-range > values (that cannot be represented by int max_loop) are ignored. > > Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> > Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> > --- > drivers/block/loop.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 102d79575895..acdd028ed486 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -2289,7 +2289,17 @@ module_exit(loop_exit); > #ifndef MODULE > static int __init max_loop_setup(char *str) > { > - max_loop = simple_strtol(str, NULL, 0); > + long max_loop_long = 0; > + > + /* > + * Range check for max_loop: negative values and values not > + * representable by int are ignored. > + */ > + if (kstrtol(str, 0, &max_loop_long) == 0 && Is there any specific reason to use kstrtol()? Since max_loop global variable is defined as an int, can we use kstrtoint()? > + max_loop_long >= 0 && > + max_loop_long <= INT_MAX) > + max_loop = (int) max_loop_long; > + > return 1; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code 2019-06-25 15:42 ` Chaitanya Kulkarni @ 2019-06-25 17:55 ` Florian Knauf 2019-06-25 19:24 ` Chaitanya Kulkarni 0 siblings, 1 reply; 6+ messages in thread From: Florian Knauf @ 2019-06-25 17:55 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: linux-kernel, Jens Axboe, linux-block, linux-kernel, Florian Knauf, Christian Ewert This patch removes the deprecated simple_strtol function from the option parsing logic in the loopback device driver. Instead kstrtoint is used to parse int max_loop, to ensure that input values it cannot represent are ignored. Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> --- Thank you for your feedback. There's no specific reason to use kstrtol, other than the fact that we weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.) We've amended the patch to make use of kstrtoint, which is of course much more straightforward. drivers/block/loop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 102d79575895..adfaf4ad37d1 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -2289,7 +2289,7 @@ module_exit(loop_exit); #ifndef MODULE static int __init max_loop_setup(char *str) { - max_loop = simple_strtol(str, NULL, 0); + kstrtoint(str, 0, &max_loop); return 1; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code 2019-06-25 17:55 ` [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code Florian Knauf @ 2019-06-25 19:24 ` Chaitanya Kulkarni 2019-06-29 19:46 ` Florian Knauf 0 siblings, 1 reply; 6+ messages in thread From: Chaitanya Kulkarni @ 2019-06-25 19:24 UTC (permalink / raw) To: Florian Knauf Cc: linux-kernel, Jens Axboe, linux-block, linux-kernel, Christian Ewert I believe you have tested this patch with loop testcases present in the :- https://github.com/osandov/blktests/tree/master/tests/loop. With that, looks good. Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>. On 06/25/2019 10:55 AM, Florian Knauf wrote: > This patch removes the deprecated simple_strtol function from the option > parsing logic in the loopback device driver. Instead kstrtoint is used to > parse int max_loop, to ensure that input values it cannot represent are > ignored. > > Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> > Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> > --- > Thank you for your feedback. > > There's no specific reason to use kstrtol, other than the fact that we > weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.) > > We've amended the patch to make use of kstrtoint, which is of course much > more straightforward. > > drivers/block/loop.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 102d79575895..adfaf4ad37d1 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -2289,7 +2289,7 @@ module_exit(loop_exit); > #ifndef MODULE > static int __init max_loop_setup(char *str) > { > - max_loop = simple_strtol(str, NULL, 0); > + kstrtoint(str, 0, &max_loop); > return 1; > } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code 2019-06-25 19:24 ` Chaitanya Kulkarni @ 2019-06-29 19:46 ` Florian Knauf 2019-07-02 5:04 ` Chaitanya Kulkarni 0 siblings, 1 reply; 6+ messages in thread From: Florian Knauf @ 2019-06-29 19:46 UTC (permalink / raw) To: Chaitanya Kulkarni Cc: linux-kernel, Jens Axboe, linux-block, linux-kernel, Christian Ewert [-- Attachment #1: Type: text/plain, Size: 2164 bytes --] I have now, on the latest staging master (test log attached, everything green), and also learned a lesson about looking more thoroughly for automated test cases. That's a mea culpa, I suppose. :P Before this I'd only found the Linux Test Project, which (if I'm not mistaken) contains tests that use loopback devices but no tests that specifically test the loopback driver itself. Given the small scope of the change, we then considered it sufficient to test manually that the loop device still worked and that the max_loop parameter was handled correctly. Of course, the blktests way is better. Thanks for taking the time to answer and review. Am 25.06.19 um 21:24 schrieb Chaitanya Kulkarni: > I believe you have tested this patch with loop testcases present in the > :- https://github.com/osandov/blktests/tree/master/tests/loop. > > With that, looks good. > > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>. > > On 06/25/2019 10:55 AM, Florian Knauf wrote: >> This patch removes the deprecated simple_strtol function from the option >> parsing logic in the loopback device driver. Instead kstrtoint is used to >> parse int max_loop, to ensure that input values it cannot represent are >> ignored. >> >> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> >> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> >> --- >> Thank you for your feedback. >> >> There's no specific reason to use kstrtol, other than the fact that we >> weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.) >> >> We've amended the patch to make use of kstrtoint, which is of course much >> more straightforward. >> >> drivers/block/loop.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 102d79575895..adfaf4ad37d1 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -2289,7 +2289,7 @@ module_exit(loop_exit); >> #ifndef MODULE >> static int __init max_loop_setup(char *str) >> { >> - max_loop = simple_strtol(str, NULL, 0); >> + kstrtoint(str, 0, &max_loop); >> return 1; >> } >> >> > [-- Attachment #2: check.log --] [-- Type: text/x-log, Size: 1370 bytes --] loop/001 (scan loop device partitions) runtime 0,401s ... loop/001 (scan loop device partitions) [passed] runtime 0,401s ... 0,269s loop/002 (try various loop device block sizes) runtime 0,142s ... loop/002 (try various loop device block sizes) [passed] runtime 0,142s ... 0,148s loop/003 (time opening and closing an unbound loop device) runtime 0,047s ... loop/003 (time opening and closing an unbound loop device) [passed] runtime 0,047s ... 0,052s loop/004 (combine loop direct I/O mode and a custom block size) runtime 0,382s ... loop/004 (combine loop direct I/O mode and a custom block size) [passed] runtime 0,382s ... 0,383s loop/005 (call LOOP_GET_STATUS{,64} with a NULL arg) runtime 0,024s ... loop/005 (call LOOP_GET_STATUS{,64} with a NULL arg) [passed] runtime 0,024s ... 0,025s loop/006 (change loop backing file while creating/removing another loop device) runtime 31,071s ... loop/006 (change loop backing file while creating/removing another loop device) [passed] runtime 31,071s ... 31,050s loop/007 (update loop device capacity with filesystem) runtime 0,417s ... loop/007 (update loop device capacity with filesystem) [passed] runtime 0,417s ... 0,351s ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code 2019-06-29 19:46 ` Florian Knauf @ 2019-07-02 5:04 ` Chaitanya Kulkarni 0 siblings, 0 replies; 6+ messages in thread From: Chaitanya Kulkarni @ 2019-07-02 5:04 UTC (permalink / raw) To: Florian Knauf Cc: linux-kernel, Jens Axboe, linux-block, linux-kernel, Christian Ewert You are welcome. > On Jun 29, 2019, at 12:46 PM, Florian Knauf <florian.knauf@stud.uni-hannover.de> wrote: > > I have now, on the latest staging master (test log attached, everything green), and also learned a lesson about looking more thoroughly for automated test cases. That's a mea culpa, I suppose. :P > > Before this I'd only found the Linux Test Project, which (if I'm not mistaken) contains tests that use loopback devices but no tests that specifically test the loopback driver itself. Given the small scope of the change, we then considered it sufficient to test manually that the loop device still worked and that the max_loop parameter was handled correctly. Of course, the blktests way is better. > > Thanks for taking the time to answer and review. > >> Am 25.06.19 um 21:24 schrieb Chaitanya Kulkarni: >> I believe you have tested this patch with loop testcases present in the >> :- https://github.com/osandov/blktests/tree/master/tests/loop. >> With that, looks good. >> Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>. >>> On 06/25/2019 10:55 AM, Florian Knauf wrote: >>> This patch removes the deprecated simple_strtol function from the option >>> parsing logic in the loopback device driver. Instead kstrtoint is used to >>> parse int max_loop, to ensure that input values it cannot represent are >>> ignored. >>> >>> Signed-off-by: Florian Knauf <florian.knauf@stud.uni-hannover.de> >>> Signed-off-by: Christian Ewert <christian.ewert@stud.uni-hannover.de> >>> --- >>> Thank you for your feedback. >>> >>> There's no specific reason to use kstrtol, other than the fact that we >>> weren't yet aware that kstrtoint exists. (We're new at this, I'm afraid.) >>> >>> We've amended the patch to make use of kstrtoint, which is of course much >>> more straightforward. >>> >>> drivers/block/loop.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >>> index 102d79575895..adfaf4ad37d1 100644 >>> --- a/drivers/block/loop.c >>> +++ b/drivers/block/loop.c >>> @@ -2289,7 +2289,7 @@ module_exit(loop_exit); >>> #ifndef MODULE >>> static int __init max_loop_setup(char *str) >>> { >>> - max_loop = simple_strtol(str, NULL, 0); >>> + kstrtoint(str, 0, &max_loop); >>> return 1; >>> } >>> >>> > <check.log> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-02 5:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-25 11:40 [PATCH] drivers/block/loop: Remove deprecated function, range check for max_loop Florian Knauf 2019-06-25 15:42 ` Chaitanya Kulkarni 2019-06-25 17:55 ` [PATCH v2] drivers/block/loop: Replace deprecated function in option parsing code Florian Knauf 2019-06-25 19:24 ` Chaitanya Kulkarni 2019-06-29 19:46 ` Florian Knauf 2019-07-02 5:04 ` Chaitanya Kulkarni
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).