lkml.org 
[lkml]   [2015]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/2] [media] mantis: Move jump label to activate dead code
Thanks for the review, Dan!

On Mon, Feb 16, 2015 at 10:04 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> 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.

I will do that.


> 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.

In the next version of the patch, I will change the names of the goto
labels accordingly. I will add the mantis_dvb_exit() call as well.


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

Ok, I will fold the second patch into the first one replacing the goto
label names with more appropriate ones. In the first patch I will just
delete the dead code and change the jump labels to be more
descriptive.

I will leave for holidays at the end of the week so it's probably
better if I wait with sending the new version of the patch until I am
back.


Thanks again,

Silvan


\
 
 \ /
  Last update: 2015-02-17 11:21    [W:0.162 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site