linux-kernel - Re: [PATCH 1/2] [media] mantis: Move jump label to activate dead code

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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 16 Feb 2015 12:04:08 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Silvan Jegen <s.jegen@...il.com>
Cc:	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] [media] mantis: Move jump label to activate dead code

On Sun, Feb 15, 2015 at 01:11:04PM +0100, Silvan Jegen wrote:
> diff --git a/drivers/media/pci/mantis/mantis_cards.c b/drivers/media/pci/mantis/mantis_cards.c
> index 801fc55..e566061 100644
> --- a/drivers/media/pci/mantis/mantis_cards.c
> +++ b/drivers/media/pci/mantis/mantis_cards.c
> @@ -215,10 +215,11 @@ static int mantis_pci_probe(struct pci_dev *pdev,
>  		dprintk(MANTIS_ERROR, 1, "ERROR: Mantis DVB initialization failed <%d>", err);
>  		goto fail4;
>  	}
> +
>  	err = mantis_uart_init(mantis);
>  	if (err < 0) {
>  		dprintk(MANTIS_ERROR, 1, "ERROR: Mantis UART initialization failed <%d>", err);
> -		goto fail6;
> +		goto fail5;
>  	}
>  
>  	devs++;
> @@ -226,10 +227,10 @@ static int mantis_pci_probe(struct pci_dev *pdev,
>  	return err;
>  
>  
> +fail5:
>  	dprintk(MANTIS_ERROR, 1, "ERROR: Mantis UART exit! <%d>", err);
>  	mantis_uart_exit(mantis);
>  
> -fail6:
>  fail4:
>  	dprintk(MANTIS_ERROR, 1, "ERROR: Mantis DMA exit! <%d>", err);
>  	mantis_dma_exit(mantis);

This patch isn't right, I'm afraid.  The person who wrote this driver
deliberately added some dead error handling code in case we change it
later and need to activate it.  It's an ugly thing to do because it
causes static checker warnings, and confusion, and, in real life, then
we are not ever going to need to activate it.  It's defensive
programming but we don't do defensive programming.
http://lwn.net/Articles/604813/  Just delete this dead code.

Also this code uses GW-BASIC style numbered gotos.  So ugly!  The label
names should be based on what the label location does.  Eg
"err_uart_exit", "err_dma_exit".  I have written an essay about label
names:  https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

In theory, we should be calling mantis_dvb_exit() if mantis_uart_init()
fails.  In reality, it can't fail but it's still wrong to leave that
out.

These patches would be easier to review if you just folded them into one
patch.  Call it "fix error error handling" or something.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/