From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755883AbbESRzu (ORCPT ); Tue, 19 May 2015 13:55:50 -0400 Received: from mail-pd0-f176.google.com ([209.85.192.176]:35086 "EHLO mail-pd0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755688AbbESRzq (ORCPT ); Tue, 19 May 2015 13:55:46 -0400 Date: Tue, 19 May 2015 10:55:32 -0700 From: Brian Norris To: Boaz Harrosh Cc: Jens Axboe , linux-kernel@vger.kernel.org, Christoph Hellwig Subject: Re: [RFC] block: remove never-modified global variable Message-ID: <20150519175532.GW11598@ld-irv-0074> References: <1431990532-7999-1-git-send-email-computersforpeace@gmail.com> <555AE3F0.5010109@plexistor.com> <555AE7A0.9040505@plexistor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <555AE7A0.9040505@plexistor.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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