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

Jonathan Corbet
2016-08-22T14:16:24Z

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