From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755971AbcHVOXK (ORCPT ); Mon, 22 Aug 2016 10:23:10 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:38740 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754794AbcHVOXI (ORCPT ); Mon, 22 Aug 2016 10:23:08 -0400 Date: Mon, 22 Aug 2016 17:23:04 +0300 From: Dan Carpenter To: "Michael S. Tsirkin" Cc: linux-kernel@vger.kernel.org, Julia Lawall , Jonathan Corbet , Jason Wang , linux-doc@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] CodingStyle: add some more error handling guidelines Message-ID: <20160822142304.GD4129@mwanda> References: <1471874251-7721-1-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1471874251-7721-1-git-send-email-mst@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 22, 2016 at 04:57:46PM +0300, Michael S. Tsirkin wrote: > commit commit ea04036032edda6f771c1381d03832d2ed0f6c31 ("CodingStyle: > add some more error handling guidelines") suggests never naming goto > labels after the goto location - that is the error that is handled. > > But it's actually pretty common and IMHO it's a reasonable style > provided each error gets its own label, and each label comes after the > matching cleanup: > > foo = kmalloc(SIZE, GFP_KERNEL); > if (!foo) > goto err_foo; Come-from labels are a common anti-pattern. The don't add any information if you are reading a function from start to end/top to bottom. Imagine if we named all functions after then functions which called them. This is the same concept. What does goto err_foo tell you? Nothing... But goto err_free_bar tells you exactly what it does. Creating a new label for each goto means you can search easily, I suppose, but jumping down to the bottom of the function and then back up disrupts the flow. We're not really using the name itself in that case, we're just treating the text as a opaque search string and not meaningful for its own sake. I see a lot of error handling bugs where people get confused or often they just decide that error handling is too complicated and they leave it out. But error handling is really simple to write correctly if you follow a few simple rules. 1) Don't just use one out label for everything. Doing multiple things makes the code more complicated and bug prone. 2) Don't free things which haven't been allocated. 3) Unwind in the reverse order that you allocated things. 4) Use meaningful names which tell what the goto does. 5) If there is an if statement in allocation code, then put an mirror if statement in the unwind code. regards, dan carpenter