From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755187AbdJSO1i (ORCPT ); Thu, 19 Oct 2017 10:27:38 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:47941 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753752AbdJSO1f (ORCPT ); Thu, 19 Oct 2017 10:27:35 -0400 Date: Thu, 19 Oct 2017 17:26:26 +0300 From: Dan Carpenter To: Michal =?iso-8859-1?Q?Such=E1nek?= Cc: SF Markus Elfring , linux-integrity@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kernel-janitors@vger.kernel.org, Stefan Berger , Nayna Jain , Jerry Snitselaar , LKML , Jarkko Sakkinen , Jason Gunthorpe , Corentin Labbe , Kenneth Goldman , Paul Mackerras , Peter =?iso-8859-1?Q?H=FCwe?= , Andy Shevchenko Subject: Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection Message-ID: <20171019142626.gwckkdbz7hsigakx@mwanda> References: <1d3516a2-a8e6-9e95-d438-f115fac84c7f@users.sourceforge.net> <09a2c3a1-1b10-507d-a866-258b570f6da1@users.sourceforge.net> <20171019135632.4af42743@kitsune.suse.cz> <9327cc83-448e-8404-af00-73f6409e269e@users.sourceforge.net> <20171019144655.7d06111c@kitsune.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171019144655.7d06111c@kitsune.suse.cz> User-Agent: NeoMutt/20170113 (1.7.2) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Date: Thu, 19 Oct 2017 14:26:26 +0000 Subject: Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection Message-Id: <20171019142626.gwckkdbz7hsigakx@mwanda> List-Id: References: <1d3516a2-a8e6-9e95-d438-f115fac84c7f@users.sourceforge.net> <09a2c3a1-1b10-507d-a866-258b570f6da1@users.sourceforge.net> <20171019135632.4af42743@kitsune.suse.cz> <9327cc83-448e-8404-af00-73f6409e269e@users.sourceforge.net> <20171019144655.7d06111c@kitsune.suse.cz> In-Reply-To: <20171019144655.7d06111c@kitsune.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Michal =?iso-8859-1?Q?Such=E1nek?= Cc: SF Markus Elfring , linux-integrity@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kernel-janitors@vger.kernel.org, Stefan Berger , Nayna Jain , Jerry Snitselaar , LKML , Jarkko Sakkinen , Jason Gunthorpe , Corentin Labbe , Kenneth Goldman , Paul Mackerras , Peter =?iso-8859-1?Q?H=FCwe?= , Andy Shevchenko 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