I found two issues regarding goto labels in the mantis driver
when checking a smatch warning and addressed them in two separate
patches. Please be aware that these patches have only been compile-tested
since I do not have access to the corresponding hardware.
Silvan Jegen (2):
[media] mantis: Move jump label to activate dead code
[media] mantis: Use correct goto labels for cleanup on error
drivers/media/pci/mantis/mantis_cards.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--
2.2.2
Due to a misplaced goto label mantis_uart_exit is never called. Adjusting
the label position (while correcting its numbering) changes this.
This issue was found using the smatch static checker.
Signed-off-by: Silvan Jegen <[email protected]>
---
drivers/media/pci/mantis/mantis_cards.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
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);
--
2.2.2
After calling mantis_pci_init we have to jump to fail2 in order to call
the corresponding mantis_pci_exit. Similarly, after calling mantis_get_mac
we have already called mantis_pci_init and mantis_i2c_init so we need
to jump to fail3 if we want to call the corresponding exit functions.
Signed-off-by: Silvan Jegen <[email protected]>
---
drivers/media/pci/mantis/mantis_cards.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/mantis/mantis_cards.c b/drivers/media/pci/mantis/mantis_cards.c
index e566061..71497d8 100644
--- a/drivers/media/pci/mantis/mantis_cards.c
+++ b/drivers/media/pci/mantis/mantis_cards.c
@@ -183,7 +183,7 @@ static int mantis_pci_probe(struct pci_dev *pdev,
err = mantis_pci_init(mantis);
if (err) {
dprintk(MANTIS_ERROR, 1, "ERROR: Mantis PCI initialization failed <%d>", err);
- goto fail1;
+ goto fail2;
}
err = mantis_stream_control(mantis, STREAM_TO_HIF);
@@ -201,7 +201,7 @@ static int mantis_pci_probe(struct pci_dev *pdev,
err = mantis_get_mac(mantis);
if (err < 0) {
dprintk(MANTIS_ERROR, 1, "ERROR: Mantis MAC address read failed <%d>", err);
- goto fail2;
+ goto fail3;
}
err = mantis_dma_init(mantis);
--
2.2.2
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
Thanks for the review, Dan!
On Mon, Feb 16, 2015 at 10:04 AM, Dan Carpenter
<[email protected]> 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