linux-kernel - Re: [PATCH] mtd: Replace if and BUG with BUG

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: <CAAEAJfBgR61_UqFBW4JinkE4wSRzCqNg95PxBcaXrhEZS=2grg@mail.gmail.com>
Date:	Mon, 30 May 2016 19:19:00 -0300
From:	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
To:	Amitoj Kaur Chawla <amitoj1606@...il.com>
Cc:	David Woodhouse <dwmw2@...radead.org>,
	Brian Norris <computersforpeace@...il.com>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	julia.lawall@...6.fr
Subject: Re: [PATCH] mtd: Replace if and BUG with BUG_ON

Hi Amitoj,

Thanks for your patch.

On 28 May 2016 at 13:41, Amitoj Kaur Chawla <amitoj1606@...il.com> wrote:
> Replace if condition and BUG() with a BUG_ON having the conditional
> expression of the if statement as argument.
>

We usually want commit messages that tell us *why* you are doing the
change: what are you fixing, or what are you improving, and what
possible side-effects it may have.

This commit log explains what the code does, but we can clearly see
that, so it's not useful.

> The Coccinelle semantic patch used to make this change is as follows:
> @@ expression E,f; @@
>
> (
>   if (<+... f(...) ...+>) { BUG(); }
> |
> - if (E) { BUG(); }
> + BUG_ON(E);
> )
>
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@...il.com>
> ---
>  drivers/mtd/ssfdc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/ssfdc.c b/drivers/mtd/ssfdc.c
> index daf82ba..41b13d1 100644
> --- a/drivers/mtd/ssfdc.c
> +++ b/drivers/mtd/ssfdc.c
> @@ -380,8 +380,7 @@ static int ssfdcr_readsect(struct mtd_blktrans_dev *dev,
>                 " block_addr=%d\n", logic_sect_no, sectors_per_block, offset,
>                 block_address);
>
> -       if (block_address >= ssfdc->map_len)
> -               BUG();
> +       BUG_ON(block_address >= ssfdc->map_len);
>

I don't want to be rude, but I wonder if there's any value at all in
such a patch. It barely improves readability, it barely reduces the
LoC, yet it consumes developer time, maintainer time, and changes git
per-line authorship (used in git blame).

I'm not complaining about *this* particular patch, but rather about
these kind of supposedly clean-up patches.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar