linux-kernel - Re: [PATCH] CodingStyle: add some more error handling guidelines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160822081617.386db8cd@lwn.net>
Date:   Mon, 22 Aug 2016 08:16:17 -0600
From:   Jonathan Corbet <corbet@....net>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Julia Lawall <julia.lawall@...6.fr>,
        Jason Wang <jasowang@...hat.com>, linux-doc@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH] CodingStyle: add some more error handling guidelines

On Mon, 22 Aug 2016 16:57:46 +0300
"Michael S. Tsirkin" <mst@...hat.com> 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