Virtualization - Aug 2016 - [PATCH] CodingStyle: add some more error handling guidelines

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;

Provides some symmetry and makes it easy to add more cases as code
calling goto does not need to worry about how is the error handled.

Cc: Dan Carpenter <dan.carpenter at oracle.com>
Cc: Julia Lawall <julia.lawall at lip6.fr>
Cc: Jonathan Corbet <corbet at lwn.net>
---
 tools/virtio/ringtest/main.h |  4 +++-
 Documentation/CodingStyle    | 44 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 3 deletions(-)

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
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index a096836..af2b5e9 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -397,8 +397,7 @@ cleanup needed then just return directly.
 
 Choose label names which say what the goto does or why the goto exists.  An
 example of a good name could be "out_buffer:" if the goto frees
"buffer".  Avoid
-using GW-BASIC names like "err1:" and "err2:".  Also
don't name them after the
-goto location like "err_kmalloc_failed:"
+using GW-BASIC names like "err1:" and "err2:".
 
 The rationale for using gotos is:
 
@@ -440,6 +439,47 @@ A common type of bug to be aware of is "one err
bugs" which look like this:
 The bug in this code is that on some exit paths "foo" is NULL. 
Normally the
 fix for this is to split it up into two error labels "err_bar:" and
"err_foo:".
 
+Note that labels normally come before the appropriate cleanups:
+
+		foo = kmalloc(SIZE, GFP_KERNEL); 
+		if (!foo)
+			goto out;
+
+		foo->bar = kmalloc(SIZE, GFP_KERNEL); 
+		if (!foo->bar)
+			goto out_foo;
+		...
+		if (err)
+			goto out_bar;
+
+	out_bar:
+		kfree(foo->bar);
+
+	out_foo:
+		kfree(foo);
+
+	out:
+		return ret;
+
+If labels are named after the goto location (or error that was detected), they
+come after the matching cleanup code:
+
+		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;
 
 		Chapter 8: Commenting
 
-- 
MST

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

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

On Mon, Aug 22, 2016 at 08:16:17AM -0600, Jonathan Corbet
wrote:
> On Mon, 22 Aug 2016 16:57:46 +0300
> "Michael S. Tsirkin" <mst at 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.
The point is really naming label for the part of init that failed
(and so needs to be skipped), rather than the part that will run.
Adding empty lines is not the point - does it look better like this?


                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 don't know whether there are examples outside code that
I wrote myself, e.g. in vhost_dev_set_owner. I find
that it's helpful since it avoids churn if more
allocations are added.

> > 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
Ouch, that's unrelated, sorry.

-- 
MST

"Michael S. Tsirkin" <mst at redhat.com> 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.

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.


Bj?rn

On Tue, Aug 23, 2016 at 01:03:15PM +0200, Bj?rn Mork
wrote:
> "Michael S. Tsirkin" <mst at redhat.com> 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