block: remove never-modified global variable
RFC 1/1 block: remove never-modified global variable
AFAICT, 'warn_no_part' never takes (and never has? at least, not in the git history) taken a value besides 1. It also has a disgruntled warning comment next to it, suggesting it shouldn't be there at all.
---
Only compile tested for now, as it's trivial. Just an RFC, since I don't really understand the context of why this (or the comment) is here in the first place. I just ran across this in code reading.
block/partitions/amiga.c | 10 ++++------ block/partitions/check.c | 7 ++----- block/partitions/check.h | 2 -- 3 files changed, 6 insertions(+), 13 deletions(-)
block/partitions/amiga.c +4 -6
@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state) goto rdb_done; data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read RDB block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read RDB block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }
@@ -84,9 +83,8 @@ int amiga_partition(struct parsed_partitions *state) blk *= blksize;/* Read in terms partition table understands */ data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read partition block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read partition block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }
block/partitions/check.c +2 -5
@@ -36,8 +36,6 @@#include "sysv68.h" #include "cmdline.h" -int warn_no_part =1 ;/*This is ugly: should make genhd removable media aware*/ -static int (*check_part[])(struct parsed_partitions *) = {/* * Probe partition formats with tables at disk address 0
@@ -185,9 +183,8 @@ check_partition(struct gendisk *hd, struct block_device *bdev)/* The partition is unrecognized. So report I/O errors if there were any */ res = err;if (res) { -if (warn_no_part) -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); +strlcat (state->pp_buf, +" unable to read partition table\n" ,PAGE_SIZE );printk (KERN_INFO "%s" , state->pp_buf); }
block/partitions/check.h +0 -2
@@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size) } } -extern int warn_no_part; -
Signature · 1 line
1.9.1
Quoted 7 lines · Brian Norris
AFAICT, '
warn_no_part' never takes (and never has? at least, not in the git history) taken a value besides 1. It also has a disgruntled warning comment next to it, suggesting it shouldn't be there at all.Signed-off-by Brian Norris <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Boaz Harrosh <[email protected]>
I have also tested it by returning error from read of sector zero. And the print prints. Of course. No one ever turns it off.
Some comments
Quoted 53 lines
Cc Jens Axboe <[email protected]>---
Only compile tested for now, as it's trivial. Just an RFC, since I don't really understand the context of why this (or the comment) is here in the first place. I just ran across this in code reading.block/partitions/amiga.c | 10 ++++------ block/partitions/check.c | 7 ++----- block/partitions/check.h | 2 -- 3 files changed, 6 insertions(+), 13 deletions(-)block/partitions/amiga.c +4
-6@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state) goto rdb_done; data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read RDB block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read RDB block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }@@ -84,9 +83,8 @@ int amiga_partition(struct parsed_partitions *state) blk *= blksize;/* Read in terms partition table understands */ data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read partition block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read partition block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }block/partitions/check.c +0
-2@@ -36,8 +36,6 @@#include "sysv68.h" #include "cmdline.h" -int warn_no_part =1 ;/*This is ugly: should make genhd removable media aware*/ -static int (*check_part[])(struct parsed_partitions *) = {/* * Probe partition formats with tables at disk address 0
block/partitions/check.c check_partition(struct gendisk *hd, struct block_device *bdev) · line 187
Quoted 8 lines
block/partitions/check.c +2
-3@@ -185,9 +183,8 @@ check_partition(struct gendisk *hd, struct block_device *bdev)/* The partition is unrecognized. So report I/O errors if there were any */ res = err;if (res) { -if (warn_no_part) -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); +strlcat (state->pp_buf, +" unable to read partition table\n" ,PAGE_SIZE );
OK I admit this was kind of very dumb before, If theoretically warn_no_part would be false then the system would print "disk_name:\n" and nothing else.
But specially now that you are unconditionally printing it. It is better to just combine the two statements. See suggested patch below:
Quoted 9 lines
block/partitions/check.c +0
-0printk (KERN_INFO "%s" , state->pp_buf); }block/partitions/check.h +0
-0
block/partitions/check.h put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size) · line 54
Quoted 5 lines
block/partitions/check.h +0
-2@@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size) } } -extern int warn_no_part; -
block/partitions/check.c +3 -5
@@ -182,11 +182,9 @@ check_partition(struct gendisk *hd, struct block_device *bdev)if (err)/* The partition is unrecognized. So report I/O errors if there were any */ res = err; -if (res) { -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); -printk (KERN_INFO "%s" , state->pp_buf); - } +if (res) +printk (KERN_INFO "%s unable to read partition table\n" , + state->pp_buf);free_page ((unsigned long )state->pp_buf);free_partitions (state);
Thanks
Boaz
On 05/19/2015 10:19 AM, Boaz Harrosh wrote:
<>
Quoted 3 lines
But specially now that you are unconditionally printing it. It is better to just combine the two statements. See suggested patch below:
Actually we can even do better:
block/partitions/check.c +0 -0
@@ -160,35 +160,31 @@ check_partition(struct gendisk *hd, struct block_device *bdev) i = res = err =0 ;while (!res && check_part[i]) {memset (state->parts,0 , state->limit *sizeof (state->parts[0 ])); res = check_part[i++](state);if (res <0 ) {/* We have hit an I/O error which we don't report now. * But record it, and let the others do their job. */ err = res; res =0 ; } }
boaz> When we exit the loop res can only be (res >= 0)
if (res > 0) {
printk(KERN_INFO "%s", state->pp_buf);
free_page((unsigned long)state->pp_buf); return state;
boaz> When bigger we exit here
}
if (state->access_beyond_eod)
err = -ENOSPC;
boaz> So by this stage res can only be == 0. So we should just do:
if (err)
/* The partition is unrecognized. So report I/O errors if there were any */
- res = err;
- if (res) {
- strlcat(state->pp_buf,
- " unable to read partition table\n", PAGE_SIZE);
- printk(KERN_INFO "%s", state->pp_buf);
- }
+ printk(KERN_INFO "%s unable to read partition table\n",
+ state->pp_buf);
free_page((unsigned long)state->pp_buf);
free_partitions(state);
return ERR_PTR(res);
}
----
Here is a proper diff:
----
block/partitions/check.c +2 -6
@@ -181,12 +181,8 @@ check_partition(struct gendisk *hd, struct block_device *bdev) err = -ENOSPC ;if (err)/* The partition is unrecognized. So report I/O errors if there were any */ - res = err; -if (res) { -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); -printk (KERN_INFO "%s" , state->pp_buf); - } +printk (KERN_INFO "%s unable to read partition table\n" , + state->pp_buf);free_page ((unsigned long )state->pp_buf);free_partitions (state);
Thanks
Boaz
Quoted 46 lines · Boaz Harrosh
On 05/19/2015 10:19 AM, Boaz Harrosh wrote:
<>Quoted 3 lines
But specially now that you are unconditionally printing it. It is better to just combine the two statements. See suggested patch below:
Actually we can even do better:
block/partitions/check.c +0
-0@@ -160,35 +160,31 @@ check_partition(struct gendisk *hd, struct block_device *bdev) i = res = err =0 ;while (!res && check_part[i]) {memset (state->parts,0 , state->limit *sizeof (state->parts[0 ])); res = check_part[i++](state);if (res <0 ) {/* We have hit an I/O error which we don't report now. * But record it, and let the others do their job. */ err = res; res =0 ; } } boaz> When we exit the loop res can only be (res >= 0) if (res > 0) {printk (KERN_INFO "%s" , state->pp_buf);free_page ((unsigned long )state->pp_buf);return state;boaz> When bigger we exit here
block/partitions/check.c +0
-0}if (state->access_beyond_eod) err = -ENOSPC ;boaz> So by this stage res can only be == 0. So we should just do:
if (err)
/* The partition is unrecognized. So report I/O errors if there were any */
block/partitions/check.c check_partition(struct gendisk *hd, struct block_device *bdev) · line 184
Quoted 1 line
block/partitions/check.c +0
-1- res = err;
^^ You don't want to kill this line, since we still return ERR_PRT(res). But otherwise, I think you're right.
Quoted 14 lines
block/partitions/check.c +2
-6-if (res) { -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); -printk (KERN_INFO "%s" , state->pp_buf); - } +printk (KERN_INFO "%s unable to read partition table\n" , + state->pp_buf);free_page ((unsigned long )state->pp_buf);free_partitions (state);return ERR_PTR (res); } ----
Quoted 1 line
Here is a proper diff:
I can send out a full patch, squashing my original + your fixup + my fixup, if that sounds OK.
Brian
Quoted 18 lines · Brian Norris
block/partitions/check.c check_partition(struct gendisk *hd, struct block_device *bdev) · line 184
Quoted 5 lines · Boaz Harrosh
boaz> So by this stage res can only be == 0. So we should just do:
if (err)
/* The partition is unrecognized. So report I/O errors if there were any */block/partitions/check.c +0
-1- res = err;^^ You don't want to kill this line, since we still return
ERR_PRT(res). But otherwise, I think you're right.Quoted 8 lines
block/partitions/check.c +2
-5-if (res) { -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); -printk (KERN_INFO "%s" , state->pp_buf); - } +printk (KERN_INFO "%s unable to read partition table\n" , + state->pp_buf);
Quoted 3 lines
block/partitions/check.c check_partition(struct gendisk *hd, struct block_device *bdev)
Quoted 3 lines
block/partitions/check.c +0
-0free_page ((unsigned long )state->pp_buf);free_partitions (state);return ERR_PTR (res);
Or rather, just make the above line:
return ERR_PTR(err);
Quoted 1 line
Quoted 1 line
}
Brian
Quoted 1 line · Brian Norris
On Tue, May 19, 2015 at 10:55:32AM -0700, Brian Norris wrote:
<>
Quoted 5 lines
Or rather, just make the above line:
return ERR_PTR(err);
Yes my thoughts too, thanks sorry about that.
Thanks
Boaz
Quoted 4 lines
Quoted 1 line
Quoted 1 line
}
Brian
PATCH v2 1/1 block: remove never-modified global variable
AFAICT, 'warn_no_part' never takes (and never has? at least, not in the git history) taken a value besides 1. It also has a disgruntled warning comment next to it, suggesting it shouldn't be there at all.
While removing the 'warn_no_part' check, let's combine a pair of string cat/print into a single print, and reduce the err/res logic to something simpler.
---
v2:
- combine the cat/print lines
- simplify the err/res logic
block/partitions/amiga.c | 10 ++++------ block/partitions/check.c | 13 +++---------- block/partitions/check.h | 2 -- 3 files changed, 7 insertions(+), 18 deletions(-)
block/partitions/amiga.c +4 -6
@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state) goto rdb_done; data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read RDB block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read RDB block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }
@@ -84,9 +83,8 @@ int amiga_partition(struct parsed_partitions *state) blk *= blksize;/* Read in terms partition table understands */ data =read_part_sector (state, blk, §);if (!data) { -if (warn_no_part) -pr_err ("Dev %s: unable to read partition block %d\n" , -bdevname (state->bdev, b), blk); +pr_err ("Dev %s: unable to read partition block %d\n" , +bdevname (state->bdev, b), blk); res =-1 ; goto rdb_done; }
block/partitions/check.c +3 -10
@@ -36,8 +36,6 @@#include "sysv68.h" #include "cmdline.h" -int warn_no_part =1 ;/*This is ugly: should make genhd removable media aware*/ -static int (*check_part[])(struct parsed_partitions *) = {/* * Probe partition formats with tables at disk address 0
@@ -183,15 +181,10 @@ check_partition(struct gendisk *hd, struct block_device *bdev) err = -ENOSPC ;if (err)/* The partition is unrecognized. So report I/O errors if there were any */ - res = err; -if (res) { -if (warn_no_part) -strlcat (state->pp_buf, -" unable to read partition table\n" ,PAGE_SIZE ); -printk (KERN_INFO "%s" , state->pp_buf); - } +printk (KERN_INFO "%s unable to read partition table\n" , + state->pp_buf);free_page ((unsigned long )state->pp_buf);free_partitions (state); -return ERR_PTR (res); +return ERR_PTR (err); }
block/partitions/check.h +0 -2
@@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size) } } -extern int warn_no_part; -
Signature · 1 line
1.9.1