linux-kernel - Re: [PATCH 2/2] mtd: nand: Get rid of needless 'goto'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Jul 2016 20:55:21 +0200
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Andrey Smirnov <andrew.smirnov@...il.com>
Cc:	Brian Norris <computersforpeace@...il.com>,
	linux-mtd@...ts.infradead.org, Richard Weinberger <richard@....at>,
	David Woodhouse <dwmw2@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mtd: nand: Get rid of needless 'goto'

On Tue, 19 Jul 2016 11:48:04 -0700
Andrey Smirnov <andrew.smirnov@...il.com> wrote:

> On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris
> <computersforpeace@...il.com> wrote:
> > On Tue, Jul 19, 2016 at 08:41:44AM -0700, Andrey Smirnov wrote:  
> >> Using "goto" in that "switch" statement only makes it harder to follow
> >> control flow and doesn't bring any advantages. Rewrite the code to avoid
> >> using "goto".
> >>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
> >> ---
> >>  drivers/mtd/nand/nand_base.c | 13 +++++--------
> >>  1 file changed, 5 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index 57043a6..8fa5536 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -2139,18 +2139,15 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
> >>       case MTD_OPS_PLACE_OOB:
> >>       case MTD_OPS_AUTO_OOB:
> >>       case MTD_OPS_RAW:
> >> +             if (!ops->datbuf)
> >> +                     ret = nand_do_read_oob(mtd, from, ops);
> >> +             else
> >> +                     ret = nand_do_read_ops(mtd, from, ops);
> >>               break;
> >> -
> >>       default:
> >> -             goto out;
> >> +             break;
> >>       }
> >>
> >> -     if (!ops->datbuf)
> >> -             ret = nand_do_read_oob(mtd, from, ops);
> >> -     else
> >> -             ret = nand_do_read_ops(mtd, from, ops);
> >> -
> >> -out:
> >>       nand_release_device(mtd);
> >>       return ret;
> >>  }  
> >
> > The default case is really just a catch-all error case. We don't
> > actually even need the nand_get_device() call for that... can we just
> > do this instead?  
> 
> Sure, although, if you don't mind, I'd rather you used:
> 
> if (ops->mode != MTD_OPS_PLACE_OOB &&
>     ops->mode != MTD_OPS_AUTO_OOB &&
>     ops->mode != MTD_OPS_RAW)
>        return -ENOTSUPP;

Or just

	if (ops->mode < MTD_OPS_PLACE_OOB || ops->mode > MTD_OPS_RAW)
		return -ENOTSUPP;

Anyway, I'm fine with all different versions as long as you don't take
the nand lock if the mode is incorrect, so I'll let Brian decide.

> 
> instead of the switch statement, IMHO, this way it is more obvious
> that this codepath is just arguments correctness checking.
> 
> Andrey