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]>
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(-)
diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 2b13533d60a2..9dbf1cec7152 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -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;
}
diff --git a/block/partitions/check.c b/block/partitions/check.c
index 16118d11dbfc..513c601b7874 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -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);
}
diff --git a/block/partitions/check.h b/block/partitions/check.h
index eade17ea910b..e09fac216adc 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size)
}
}
-extern int warn_no_part;
-
--
1.9.1
On 05/19/2015 02:08 AM, Brian Norris wrote:
> 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]>
Reviewed-by: 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
> 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(-)
>
> diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
> index 2b13533d60a2..9dbf1cec7152 100644
> --- a/block/partitions/amiga.c
> +++ b/block/partitions/amiga.c
> @@ -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;
> }
> diff --git a/block/partitions/check.c b/block/partitions/check.c
> index 16118d11dbfc..513c601b7874 100644
> --- a/block/partitions/check.c
> +++ b/block/partitions/check.c
> @@ -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);
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:
> printk(KERN_INFO "%s", state->pp_buf);
> }
>
> diff --git a/block/partitions/check.h b/block/partitions/check.h
> index eade17ea910b..e09fac216adc 100644
> --- a/block/partitions/check.h
> +++ b/block/partitions/check.h
> @@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size)
> }
> }
>
> -extern int warn_no_part;
> -
>
diff --git a/block/partitions/check.c b/block/partitions/check.c
index 513c601..5fd2b7e 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -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:
<>
> 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:
diff --git a/block/partitions/check.c b/block/partitions/check.c
index 513c601..9bea23d 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -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:
----
diff --git a/block/partitions/check.c b/block/partitions/check.c
index 513c601..9bea23d 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -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
On Tue, May 19, 2015 at 10:34:56AM +0300, Boaz Harrosh wrote:
> On 05/19/2015 10:19 AM, Boaz Harrosh wrote:
> <>
> > 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:
>
> diff --git a/block/partitions/check.c b/block/partitions/check.c
> index 513c601..9bea23d 100644
> --- a/block/partitions/check.c
> +++ b/block/partitions/check.c
> @@ -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;
^^ You don't want to kill this line, since we still return ERR_PRT(res).
But otherwise, I think you're right.
> - 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:
I can send out a full patch, squashing my original + your fixup + my
fixup, if that sounds OK.
Brian
On Tue, May 19, 2015 at 10:55:32AM -0700, Brian Norris wrote:
> On Tue, May 19, 2015 at 10:34:56AM +0300, Boaz Harrosh wrote:
> > 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;
>
> ^^ You don't want to kill this line, since we still return ERR_PRT(res).
> But otherwise, I think you're right.
>
> > - 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);
Or rather, just make the above line:
return ERR_PTR(err);
> > }
Brian
On 05/19/2015 08:58 PM, Brian Norris wrote:
> On Tue, May 19, 2015 at 10:55:32AM -0700, Brian Norris wrote:
<>
>
> Or rather, just make the above line:
>
> return ERR_PTR(err);
>
Yes my thoughts too, thanks sorry about that.
Thanks
Boaz
>>> }
>
> Brian
>
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.
Signed-off-by: Brian Norris <[email protected]>
Reviewed-by: Boaz Harrosh <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Jens Axboe <[email protected]>
---
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(-)
diff --git a/block/partitions/amiga.c b/block/partitions/amiga.c
index 2b13533d60a2..9dbf1cec7152 100644
--- a/block/partitions/amiga.c
+++ b/block/partitions/amiga.c
@@ -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;
}
diff --git a/block/partitions/check.c b/block/partitions/check.c
index 16118d11dbfc..2f83651692ef 100644
--- a/block/partitions/check.c
+++ b/block/partitions/check.c
@@ -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);
}
diff --git a/block/partitions/check.h b/block/partitions/check.h
index eade17ea910b..e09fac216adc 100644
--- a/block/partitions/check.h
+++ b/block/partitions/check.h
@@ -50,5 +50,3 @@ put_partition(struct parsed_partitions *p, int n, sector_t from, sector_t size)
}
}
-extern int warn_no_part;
-
--
1.9.1