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]

Message-ID: <20160719183047.GC85399@google.com>
Date:	Tue, 19 Jul 2016 11:30:47 -0700
From:	Brian Norris <computersforpeace@...il.com>
To:	Andrey Smirnov <andrew.smirnov@...il.com>
Cc:	linux-mtd@...ts.infradead.org,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	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, 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?

static int nand_read_oob(struct mtd_info *mtd, loff_t from,
			 struct mtd_oob_ops *ops)
{
	int ret;
	 
	ops->retlen = 0; 

	/* Do not allow reads past end of device */
	if (ops->datbuf && (from + ops->len) > mtd->size) {
		pr_debug("%s: attempt to read beyond end of device\n",
				__func__);
		return -EINVAL;
	}

	switch (ops->mode) {
	case MTD_OPS_PLACE_OOB:
	case MTD_OPS_AUTO_OOB:
	case MTD_OPS_RAW:
		break;

	default:
		return -ENOTSUPP;
	}

	nand_get_device(mtd, FL_READING);

	if (!ops->datbuf)
		ret = nand_do_read_oob(mtd, from, ops);
	else
		ret = nand_do_read_ops(mtd, from, ops);

	nand_release_device(mtd);
	return ret;
}

i.e., this diff:

Signed-off-by: Brian Norris <computersforpeace@...il.com>

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 77533f7f2429..881dbd495466 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2162,7 +2162,7 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 			 struct mtd_oob_ops *ops)
 {
-	int ret = -ENOTSUPP;
+	int ret;
 
 	ops->retlen = 0;
 
@@ -2173,8 +2173,6 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 		return -EINVAL;
 	}
 
-	nand_get_device(mtd, FL_READING);
-
 	switch (ops->mode) {
 	case MTD_OPS_PLACE_OOB:
 	case MTD_OPS_AUTO_OOB:
@@ -2182,15 +2180,16 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 		break;
 
 	default:
-		goto out;
+		return -ENOTSUPP;
 	}
 
+	nand_get_device(mtd, FL_READING);
+
 	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;
 }