Skip to contentSkip to conversation

block: remove never-modified global variable

RFC Superseded Brian Norris 2 participants 7 messages May 18, 2015 – May 21, 2015 Block layer

lore
Share & tools
lore
#0 lore raw

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.

Cc Christoph Hellwig <[email protected]>
Cc Boaz Harrosh <[email protected]>
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
index 2b13533d60a2..9dbf1cec7152 100644
@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state)
 			goto rdb_done;
 		data = read_part_sector(state, blk, &sect);
 		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, &sect);
 		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
index 16118d11dbfc..513c601b7874 100644
@@ -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
index eade17ea910b..e09fac216adc 100644
@@ -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
Reviewed-by on v1 1/1
#1 lore raw
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.

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
index 2b13533d60a2..9dbf1cec7152 100644
@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state)
 			goto rdb_done;
 		data = read_part_sector(state, blk, &sect);
 		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, &sect);
 		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
index 16118d11dbfc..513c601b7874 100644
@@ -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 -0
 		printk(KERN_INFO "%s", state->pp_buf);
 	}
 
block/partitions/check.h +0 -0
index eade17ea910b..e09fac216adc 100644

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
index 513c601..5fd2b7e 100644
@@ -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

#2 lore raw

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
index 513c601..9bea23d 100644
@@ -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
index 513c601..9bea23d 100644
@@ -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

#3 lore raw
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
index 513c601..9bea23d 100644
@@ -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

#4 lore raw
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 -0
 	free_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

#5 lore raw
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

#6 lore raw

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
index 2b13533d60a2..9dbf1cec7152 100644
@@ -41,9 +41,8 @@ int amiga_partition(struct parsed_partitions *state)
 			goto rdb_done;
 		data = read_part_sector(state, blk, &sect);
 		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, &sect);
 		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
index 16118d11dbfc..2f83651692ef 100644
@@ -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
index eade17ea910b..e09fac216adc 100644
@@ -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