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

[Silvan Jegen]

Thanks for the review, Dan!

On Mon, Feb 16, 2015 at 10:04 AM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/