From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757942AbcHWL7I (ORCPT ); Tue, 23 Aug 2016 07:59:08 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:28505 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757213AbcHWL6p (ORCPT ); Tue, 23 Aug 2016 07:58:45 -0400 Date: Tue, 23 Aug 2016 14:58:32 +0300 From: Dan Carpenter To: =?iso-8859-1?Q?Bj=F8rn?= Mork Cc: "Michael S. Tsirkin" , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Jonathan Corbet , virtualization@lists.linux-foundation.org, Julia Lawall Subject: Re: [PATCH] CodingStyle: add some more error handling guidelines Message-ID: <20160823115832.GG4129@mwanda> References: <1471874251-7721-1-git-send-email-mst@redhat.com> <87mvk3vjbg.fsf@miraculix.mork.no> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87mvk3vjbg.fsf@miraculix.mork.no> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 23, 2016 at 01:03:15PM +0200, Bjørn Mork wrote: > "Michael S. Tsirkin" writes: > > > foo = kmalloc(SIZE, GFP_KERNEL); > > if (!foo) > > goto err_foo; > > > > foo->bar = kmalloc(SIZE, GFP_KERNEL); > > if (!foo->bar) > > goto err_bar; > > ... > > > > kfree(foo->bar); > > err_bar: > > > > kfree(foo); > > err_foo: > > > > return ret; > > > I believe the CodingStyle already contain far too much personal style to > be useful as real style guide. FWIW, I prefer a single error label, at > the "cost" of additional tests in the error path: > > > foo = kmalloc(SIZE, GFP_KERNEL); > if (!foo) > goto err; > foo->bar = kmalloc(SIZE, GFP_KERNEL); > if (!foo->bar) > goto err; > ... > if (ret) > goto err; > return 0; > err: > if (foo) > kfree(foo->bar); > kfree(foo); > return ret; > > > The advantage is that I don't have to manage X different labels, > ensuring that they have the order is correct if some part of the > function is refactored etc. That tends to get too complicated for my > simple brain. And since the error path is rarely tested, complicated > equals buggy. Empirically, that style is *way* more bug prone. I call these bugs "One Err Bugs". It's one of the most common types of bugs I deal with from static analysis. The order is not hard. It's just the reverse order from how it was allocated. Hike up the mountain, then if you get stuck hike back down using the exact same path. Then at the end, you basically have written your ->remove() function so it's a bonus. > > My sample will of course trigger all those nice "optimizing the error > path" patches, but I ignore those anyway so that's not a big deal. That's not my fault. :/ I have tried over and over and over to tell that guy to stop sending patches but everyone else encourages him. I feel like it should be a rule that if you introduce bugs, you should be told to stop sending cleanup patches until you have fixed enough bugs to redeem yourself. regards, dan carpenter