Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755049AbbESHTV (ORCPT ); Tue, 19 May 2015 03:19:21 -0400 Received: from mail-wg0-f47.google.com ([74.125.82.47]:35167 "EHLO mail-wg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754104AbbESHTR (ORCPT ); Tue, 19 May 2015 03:19:17 -0400 Message-ID: <555AE3F0.5010109@plexistor.com> Date: Tue, 19 May 2015 10:19:12 +0300 From: Boaz Harrosh User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Brian Norris , Jens Axboe CC: linux-kernel@vger.kernel.org, Christoph Hellwig Subject: Re: [RFC] block: remove never-modified global variable References: <1431990532-7999-1-git-send-email-computersforpeace@gmail.com> In-Reply-To: <1431990532-7999-1-git-send-email-computersforpeace@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4539 Lines: 128 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 > Cc: Christoph Hellwig > Cc: Boaz Harrosh Reviewed-by: Boaz Harrosh 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 > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/