loop: three small cleanups
cover loop: three small cleanups
From: Tao Cui <[email protected]>
A stale kerneldoc reference, a simple_strtol() left in the max_loop setup code, and an unused function argument.
Tao Cui (3):
loop: drop a stale reference to loop_validate_size()
loop: replace simple_strtol() with kstrtoint()
loop: drop the unused argument of lo_req_flush()
drivers/block/loop.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Signature · 1 line
2.43.0
PATCH v2 1/3 loop: drop a stale reference to loop_validate_size()
From: Tao Cui <[email protected]>
The kerneldoc of loop_set_size() points readers at loop_validate_size(), which does not exist in the tree. Drop the reference.
--- drivers/block/loop.c | 3 --- 1 file changed, 3 deletions(-)
drivers/block/loop.c +0 -3
@@ -212,9 +212,6 @@ static inline void loop_update_dio(struct loop_device *lo)* loop_set_size() - sets device size and notifies userspace * @lo: struct loop_device to set the size for * @size: new size of the loop device -* -* Callers must validate that the size passed into this function fits into -* a sector_t, eg using loop_validate_size() */ static void loop_set_size (struct loop_device *lo,loff_t size) {
Signature · 1 line
2.43.0
Re: [PATCH v2 1/3] loop: drop a stale reference to loop_validate_size()
Looks good:
PATCH v2 2/3 loop: replace simple_strtol() with kstrtoint()
In reply to Tao CuiFrom: Tao Cui <[email protected]>
simple_strtol() is deprecated and swallows errors. Use kstrtoint() in the max_loop setup code; an invalid option keeps the default and now says so.
Only mark max_loop_specified when the option was parsed successfully. With simple_strtol() a garbage string yields max_loop = 0, and loop_probe()'s "max_loop_specified && max_loop" check short-circuits on 0, so legacy autoloading is not capped. Keeping max_loop at its default while still setting max_loop_specified would instead turn the default into a hard upper bound for dynamic device creation. This mirrors what max_loop_param_set_int() already does for the module parameter.
---
Changes in v2:
- move max_loop_specified = true under the success path, pointed out by an AI-assisted review of v1.
--- drivers/block/loop.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/block/loop.c +4 -1
@@ -2341,7 +2341,10 @@ module_exit(loop_exit);#ifndef MODULE static int __init max_loop_setup (char *str) { - max_loop =simple_strtol (str,NULL ,0 ); +if (kstrtoint (str,0 , &max_loop)) { +pr_warn ("loop: invalid max_loop, keeping default\n" ); +return 1 ; + }#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD max_loop_specified = true;#endif
Signature · 1 line
2.43.0
Re: [PATCH v2 2/3] loop: replace simple_strtol() with kstrtoint()
Hi Bart,
Quoted 6 lines · Tao Cui
From: Tao Cui <[email protected]>
simple_strtol()is deprecated and swallows errors. Usekstrtoint()in themax_loopsetup code; an invalid option keeps the default and now says so.
Quoted 9 lines
Only mark
max_loop_specifiedwhen the option was parsed successfully. Withsimple_strtol()a garbage string yieldsmax_loop= 0, and loop_probe()'s "max_loop_specified&&max_loop" check short-circuits on 0, so legacy autoloading is not capped. Keepingmax_loopat its default while still settingmax_loop_specifiedwould instead turn the default into a hard upper bound for dynamic device creation. This mirrors whatmax_loop_param_set_int()already does for the module parameter.
Thanks for the review!
After sending v1 I noticed a regression in this patch, so I'll follow up with a v2 of just this one (1/3 and 3/3 are unchanged and keep your Reviewed-by).
With simple_strtol() a garbage max_loop= string yields max_loop = 0, and loop_probe()'s "max_loop_specified && max_loop" check short-circuits on 0, so legacy autoloading was not capped. In v1 an invalid string keeps max_loop at its default value while max_loop_specified is still set to true, which turns the default into a hard upper bound for dynamic device creation. The v2 only marks max_loop_specified when kstrtoint() succeeds, mirroring max_loop_param_set_int():
if (kstrtoint(str, 0, &max_loop))
+ {
pr_warn("loop: invalid max_loop, keeping default\n");
+ return 1;
+ }
#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD
max_loop_specified = true;
#endif
This was pointed out by an AI-assisted review of v1, so I've left your Reviewed-by off this one for now. Could you take another look when you have a chance?
Thanks,
Tao
Quoted 26 lines
Signed-off-by Tao Cui <[email protected]>---
Changes in v2:
- movemax_loop_specified= true under the success path, pointed out by an AI-assisted review of v1.--- drivers/block/loop.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/block/loop.c +4
-1@@ -2341,7 +2341,10 @@ module_exit(loop_exit);#ifndef MODULE static int __init max_loop_setup (char *str) { - max_loop =simple_strtol (str,NULL ,0 ); +if (kstrtoint (str,0 , &max_loop)) { +pr_warn ("loop: invalid max_loop, keeping default\n" ); +return 1 ; + }#ifdef CONFIG_BLOCK_LEGACY_AUTOLOAD max_loop_specified = true;#endif
PATCH v2 3/3 loop: drop the unused argument of lo_req_flush()
In reply to Tao CuiFrom: Tao Cui <[email protected]>
lo_req_flush() never uses its request argument. Drop it.
--- drivers/block/loop.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/block/loop.c +2 -2
@@ -270,7 +270,7 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,return ret; } -static int lo_req_flush (struct loop_device *lo,struct request *rq) +static int lo_req_flush (struct loop_device *lo) {int ret =vfs_fsync (lo->lo_backing_file,0 );if (unlikely (ret && ret != -EINVAL ))
@@ -411,7 +411,7 @@ static int do_req_filebacked(struct loop_device *lo, struct request *rq)switch (req_op (rq)) {case REQ_OP_FLUSH : -return lo_req_flush (lo, rq); +return lo_req_flush (lo);case REQ_OP_WRITE_ZEROES :/* * If the caller doesn't want deallocation, call zeroout to
Signature · 1 line
2.43.0
Re: [PATCH v2 3/3] loop: drop the unused argument of lo_req_flush()
Looks good: