tpm_ibmvtpm_probe() is an example of poor names. It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:
647 rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
648 tpm_ibmvtpm_driver_name, ibmvtpm);
649 if (rc) {
650 dev_err(dev, "Error %d register irq 0x%x\n", rc, vio_dev->irq);
651 goto init_irq_cleanup;
652 }
653
654 rc = vio_enable_interrupts(vio_dev);
655 if (rc) {
656 dev_err(dev, "Error %d enabling interrupts\n", rc);
657 goto init_irq_cleanup;
658 }
Sadly, we never do call free_irq().
> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.
I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof". It's not true.
The best error handling is methodical error handling:
1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
does
5) That doesn't free anything which hasn't been allocated
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
