From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755768AbcHVOQY (ORCPT ); Mon, 22 Aug 2016 10:16:24 -0400 Received: from tex.lwn.net ([70.33.254.29]:46050 "EHLO vena.lwn.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751032AbcHVOQX (ORCPT ); Mon, 22 Aug 2016 10:16:23 -0400 Date: Mon, 22 Aug 2016 08:16:17 -0600 From: Jonathan Corbet To: "Michael S. Tsirkin" Cc: linux-kernel@vger.kernel.org, Dan Carpenter , Julia Lawall , Jason Wang , linux-doc@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] CodingStyle: add some more error handling guidelines Message-ID: <20160822081617.386db8cd@lwn.net> In-Reply-To: <1471874251-7721-1-git-send-email-mst@redhat.com> References: <1471874251-7721-1-git-send-email-mst@redhat.com> Organization: LWN.net X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 22 Aug 2016 16:57:46 +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; > > foo->bar = kmalloc(SIZE, GFP_KERNEL); > if (!foo->bar) > goto err_bar; > ... > > kfree(foo->bar); > err_bar: > > kfree(foo); > err_foo: > > return ret; Hmm, I've never encountered that style, but I've never gone looking for it either. I find it a little confusing to detach a label from the code it will run. Is this really something we want to encourage? I kind of think this one needs some acks before I can consider it. > diff --git a/tools/virtio/ringtest/main.h b/tools/virtio/ringtest/main.h > index 16917ac..e4d76c3 100644 > --- a/tools/virtio/ringtest/main.h > +++ b/tools/virtio/ringtest/main.h > @@ -80,7 +80,9 @@ extern unsigned ring_size; > > /* Is there a portable way to do this? */ > #if defined(__x86_64__) || defined(__i386__) > -#define cpu_relax() asm ("rep; nop" ::: "memory") > +#define cpu_relax() do { \ > + asm ("rep; nop" ::: "memory"); \ > +} while (0) > #else > #define cpu_relax() assert(0) > #endif This hunk seems somehow unrelated, either that or I really haven't understood the proposal :) jon