[PATCH] CodingStyle: add some more error handling guidelines

[PATCH] CodingStyle: add some more error handling guidelines

[Michael S. Tsirkin]

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@oracle.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: Jonathan Corbet <corbet@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

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

[Jonathan Corbet]

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

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

[Dan Carpenter]

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

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

[Michael S. Tsirkin]

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@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

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

[Dan Carpenter]

vhost_dev_set_owner() is an example of why come-from labels are
bad style.

devel/drivers/vhost/vhost.c
   473  /* Caller should have device mutex */
   474  long vhost_dev_set_owner(struct vhost_dev *dev)
   475  {
   476          struct task_struct *worker;
   477          int err;
   478  
   479          /* Is there an owner already? */
   480          if (vhost_dev_has_owner(dev)) {
   481                  err = -EBUSY;
   482                  goto err_mm;

What does goto err_mm do?  It's actually a do-nothing goto.  It would
be easier to read as a direct return.  Why is it called err_mm?  Because
originally the condition was:

	if (dev->mm) {
		err = -EBUSY;
		goto err_mm;
	}

We've changed the code but didn't update the label so it's slightly
confusing unless you know how vhost_dev_has_owner() is implemented.

   483          }
   484  
   485          /* No owner, become one */
   486          dev->mm = get_task_mm(current);
   487          worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
   488          if (IS_ERR(worker)) {
   489                  err = PTR_ERR(worker);
   490                  goto err_worker;
   491          }
   492  
   493          dev->worker = worker;
   494          wake_up_process(worker);        /* avoid contributing to loadavg */
   495  
   496          err = vhost_attach_cgroups(dev);
   497          if (err)
   498                  goto err_cgroup;
   499  
   500          err = vhost_dev_alloc_iovecs(dev);
   501          if (err)
   502                  goto err_cgroup;

This name doesn't make sense because it's a come-from label which is
used twice.  Some people do:

		if (err)
			goto err_iovecs;

   503  
   504          return 0;

Then they add two labels here:

	err_iovecs:
	err_cgroup:
		kthread_stop(worker);

But if you base the label name on the label location then it makes
sense.  goto stop_kthread;  goto err_mmput;.

   505  err_cgroup:
   506          kthread_stop(worker);
   507          dev->worker = NULL;
   508  err_worker:
   509          if (dev->mm)
   510                  mmput(dev->mm);
   511          dev->mm = NULL;
   512  err_mm:
   513          return err;
   514  }

regards,
dan carpenter

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

[Michael S. Tsirkin]

On Mon, Aug 22, 2016 at 09:31:40PM +0300, Dan Carpenter wrote:
> 
> vhost_dev_set_owner() is an example of why come-from labels are
> bad style.
> 
> devel/drivers/vhost/vhost.c
>    473  /* Caller should have device mutex */
>    474  long vhost_dev_set_owner(struct vhost_dev *dev)
>    475  {
>    476          struct task_struct *worker;
>    477          int err;
>    478  
>    479          /* Is there an owner already? */
>    480          if (vhost_dev_has_owner(dev)) {
>    481                  err = -EBUSY;
>    482                  goto err_mm;
> 
> What does goto err_mm do?  It's actually a do-nothing goto.  It would
> be easier to read as a direct return.  Why is it called err_mm?  Because
> originally the condition was:
> 
> 	if (dev->mm) {
> 		err = -EBUSY;
> 		goto err_mm;
> 	}
> 
> We've changed the code but didn't update the label so it's slightly
> confusing unless you know how vhost_dev_has_owner() is implemented.
> 
>    483          }
>    484  
>    485          /* No owner, become one */
>    486          dev->mm = get_task_mm(current);
>    487          worker = kthread_create(vhost_worker, dev, "vhost-%d", current->pid);
>    488          if (IS_ERR(worker)) {
>    489                  err = PTR_ERR(worker);
>    490                  goto err_worker;
>    491          }
>    492  
>    493          dev->worker = worker;
>    494          wake_up_process(worker);        /* avoid contributing to loadavg */
>    495  
>    496          err = vhost_attach_cgroups(dev);
>    497          if (err)
>    498                  goto err_cgroup;
>    499  
>    500          err = vhost_dev_alloc_iovecs(dev);
>    501          if (err)
>    502                  goto err_cgroup;
> 
> This name doesn't make sense because it's a come-from label which is
> used twice.  Some people do:
> 
> 		if (err)
> 			goto err_iovecs;
> 
>    503  
>    504          return 0;


Right and the current CodingStyle text seems to discourage this.

> Then they add two labels here:
> 
> 	err_iovecs:
> 	err_cgroup:
> 		kthread_stop(worker);

Definitely good points above, I'll fix them up.


> But if you base the label name on the label location then it makes
> sense.  goto stop_kthread;  goto err_mmput;.
> 
>    505  err_cgroup:
>    506          kthread_stop(worker);
>    507          dev->worker = NULL;
>    508  err_worker:
>    509          if (dev->mm)
>    510                  mmput(dev->mm);
>    511          dev->mm = NULL;
>    512  err_mm:
>    513          return err;
>    514  }
> 
> regards,
> dan carpenter

OK, I'll consider this, thanks!

-- 
MST

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

[Dan Carpenter]

On Mon, Aug 22, 2016 at 05:53:02PM +0300, Michael S. Tsirkin wrote:
> 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.

Naming labels after what "needs to be skipped" doesn't work.  How does
that meaning make sense for err_cgroup in vhost_dev_set_owner()?  What
needs to be skipped here?

regards,
dan carpenter

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

[Michael S. Tsirkin]

On Mon, Aug 22, 2016 at 09:50:06PM +0300, Dan Carpenter wrote:
> On Mon, Aug 22, 2016 at 05:53:02PM +0300, Michael S. Tsirkin wrote:
> > 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.
> 
> Naming labels after what "needs to be skipped" doesn't work.  How does
> that meaning make sense for err_cgroup in vhost_dev_set_owner()?  What
> needs to be skipped here?
> 
> regards,
> dan carpenter


Nothing because we are destroying the thread, so we don't need
to detach it. I guess I'm convinced it's not very consistent
at this point.

-- 
MST

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

[Bjørn Mork]

"Michael S. Tsirkin" <mst@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

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

[Dan Carpenter]

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

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

[Bjørn Mork]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hike up the mountain, then if you get stuck hike back down using the
> exact same path.

OK, I understand what you say.  I just can't resist objecting to that
example ;)

In my experience, finding the exact same path back after hiking up a
mountain is really hard. Especially if you are in enough trouble already
to get stuck. Up and down tend to look completely different.  Looking
back all the time while you go up might help, but finding your way back
by simply taking the reverse path is definitely not easy.


Bjørn

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

[Dan Carpenter]

Lol.  The mossy side of a boulder is the alloc, the non-mossy side is
the free!

:P

regards,
dan carpenter

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

[SF Markus Elfring]

> Please provide your point of view.

I would like to interpret the key word "goto" from the C programming
language a bit more here so that a better common understanding can
eventually be achieved.

Strong opinions might be floating around for the consistent naming
of jump labels. My reasoning works like the following.

This key word could also be interpreted as two items "go" and "to",
couldn't it?
How much does this variation stress its meaning in a specific direction?

Some software developers would like to express the reason about
an unexpected event at the jump source. But I guess that this approach
increases the risk for a popular story like "goto fail;", doesn't it?
I would prefer not to specify "go to failure".

So I find that there are more variants possible to stress the jump target.
Examples:
* Failure_exit
* out_memory_release
* unregister_item

Regards,
Markus

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

[Arend van Spriel]

On 12/03/14 17:00, SF Markus Elfring wrote:
>>> Which name pattern do you find more appropriate in such
>>> an use case?
>>
>> I think Dan wants the label to be descriptive about the tasks
>> needed in the exception handling itself.
>
> I would usually prefer also such a target-oriented labelling
> for the affected identifiers.
> How are the chances to express an expectation in this direction
> unambiguously for the proposed coding style update?
>
>
>> This makes sense as the exception handling steps may be reused
>> for different failures in the code.
>
> I would stress a different reason from my point of view.

I meant as apposed to using a goto-/source-oriented labelling. Please 
provide your point of view. That way the explanations given in this 
email exchange might be incorporated in the next round of the proposed 
update or at least be used as input.

Regards,
Arend

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

[SF Markus Elfring]

>> Which name pattern do you find more appropriate in such
>> an use case?
> 
> I think Dan wants the label to be descriptive about the tasks
> needed in the exception handling itself.

I would usually prefer also such a target-oriented labelling
for the affected identifiers.
How are the chances to express an expectation in this direction
unambiguously for the proposed coding style update?


> This makes sense as the exception handling steps may be reused
> for different failures in the code.

I would stress a different reason from my point of view.

Regards,
Markus

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

[Arend van Spriel]

On 12/03/14 14:24, SF Markus Elfring wrote:
>> Sorry.  I misread your email.  If the code looks like this:
>>
>> 	foo = kmalloc();
>> 	if (!foo)
>> 		goto kmalloc_failed;
>>
>> The "kmalloc_failed" doesn't add any information.
>
> I find that this such a name approach would fit to your
> expectation of a source-oriented labeling of these identifiers.
>
>
>> We can see that kmalloc failed from the context.
>
> Which name pattern do you find more appropriate in such
> an use case?

I think Dan wants the label to be descriptive about the tasks needed in 
the exception handling itself. This makes sense as the exception 
handling steps may be reused for different failures in the code.

void foo(void)
{
	if (check_a())
		goto do_bar;

	sub_foo1();

	if (checck_b())
		goto do_bar;

	sub_foo2();
	return;

do_bar:
	bar();
}

Regards,
Arend

> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe backports" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[SF Markus Elfring]

> Sorry.  I misread your email.  If the code looks like this:
> 
> 	foo = kmalloc();
> 	if (!foo)
> 		goto kmalloc_failed;
> 
> The "kmalloc_failed" doesn't add any information.

I find that this such a name approach would fit to your
expectation of a source-oriented labeling of these identifiers.


> We can see that kmalloc failed from the context.

Which name pattern do you find more appropriate in such
an use case?

Regards,
Markus

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

[Dan Carpenter]

Sorry.  I misread your email.  If the code looks like this:

	foo = kmalloc();
	if (!foo)
		goto kmalloc_failed;

The "kmalloc_failed" doesn't add any information.  We can see that
kmalloc failed from the context.

regards,
dan carpenter

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

[Dan Carpenter]

On Wed, Dec 03, 2014 at 01:52:53PM +0100, Julia Lawall wrote:
> On Wed, 3 Dec 2014, Dan Carpenter wrote:
> 
> > On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
> > >
> > > * To which source code place should the word "location" refer to?
> > >   - jump source
> > >   - jump target
> >
> > jump target.
> 
> I think you mean source?  Or it really is ambiguous.  The example was
> err_kmalloc_failed, which seems source-related.

Yeah.  You're right.  I misread Markus's email.  The goto location is
where the goto is.  The label location is where the label is.

regards,
dan carpenter

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

[SF Markus Elfring]

>> * To which source code place should the word "location" refer to?
>>   - jump source
>>   - jump target
> 
> jump target.

Thanks for the clarification of your intention.

I wonder then why I got the feedback "That is a useless thing to do."
from you yesterday.
I hope that we can still clarify our different opinions about specific
implementation details in constructive ways.

Regards,
Markus

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

[Julia Lawall]

On Wed, 3 Dec 2014, Dan Carpenter wrote:

> On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
> >
> > * To which source code place should the word "location" refer to?
> >   - jump source
> >   - jump target
>
> jump target.

I think you mean source?  Or it really is ambiguous.  The example was
err_kmalloc_failed, which seems source-related.

julia

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

[SF Markus Elfring]

>> * To which source code place should the word "location" refer to?
>>    - jump source
>>    - jump target
> 
> I think you digested the paragraph in too small bits.

I would prefer to reduce the probability for misunderstandings of the 
proposed wording a bit more.


> The term "goto location" looks synonymous to "jump source" to me.

I would interpret it differently because of the specific placement of this key word
before an other term.

Regards,
Markus

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

[Dan Carpenter]

On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
> 
> * To which source code place should the word "location" refer to?
>   - jump source
>   - jump target

jump target.

regards,
dan carpenter

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

[Arend van Spriel]

On 12/03/14 13:31, SF Markus Elfring wrote:
>> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
>> index 9f28b14..9c8a234 100644
>> --- a/Documentation/CodingStyle
>> +++ b/Documentation/CodingStyle
>> @@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
>>   locations and some common work such as cleanup has to be done.  If there is no
>>   cleanup needed then just return directly.
>>
>> -The rationale is:
>> +Choose label names which say what the goto does or why the goto exists.  An
>> +[...]  Avoid
>> +using GW-BASIC names like "err1:" and "err2:".  Also don't name them after the
>> +goto location like "err_kmalloc_failed:"
>
> I find this documentation approach not safe and clear enough so far.
>
> * How should the reference to an other programming language help in the understanding
>    of the recommended naming convention for jump labels?
>
> * To which source code place should the word "location" refer to?
>    - jump source
>    - jump target

I think you digested the paragraph in too small bits. The term "goto 
location" looks synonymous to "jump source" to me.

Regards,
Arend

> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe backports" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[SF Markus Elfring]

> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 9f28b14..9c8a234 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
>  locations and some common work such as cleanup has to be done.  If there is no
>  cleanup needed then just return directly.
>  
> -The rationale is:
> +Choose label names which say what the goto does or why the goto exists.  An
> +[...]  Avoid
> +using GW-BASIC names like "err1:" and "err2:".  Also don't name them after the
> +goto location like "err_kmalloc_failed:"

I find this documentation approach not safe and clear enough so far.

* How should the reference to an other programming language help in the understanding
  of the recommended naming convention for jump labels?

* To which source code place should the word "location" refer to?
  - jump source
  - jump target

Regards,
Markus

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

[Jonathan Corbet]

On Tue, 2 Dec 2014 10:09:02 +0100 (CET)
Julia Lawall <julia.lawall@lip6.fr> wrote:

> kmalloc actually takes two arguments.  Perhaps it would be better to show
> something that looks like a valid call.

Agreed; I took the liberty of sticking in a GFP_KERNEL as I applied the
patch with Julia's ack.

Thanks,

jon

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

[Julia Lawall]

> @@ -403,9 +408,10 @@ The rationale is:
>  int fun(int a)
>  {
>  	int result = 0;
> -	char *buffer = kmalloc(SIZE);
> +	char *buffer;
>
> -	if (buffer == NULL)
> +	buffer = kmalloc(SIZE);

kmalloc actually takes two arguments.  Perhaps it would be better to show
something that looks like a valid call.

Otherwise,

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

julia

> +	if (!buffer)
>  		return -ENOMEM;
>
>  	if (condition1) {
> @@ -413,14 +419,25 @@ int fun(int a)
>  			...
>  		}
>  		result = 1;
> -		goto out;
> +		goto out_buffer;
>  	}
>  	...
> -out:
> +out_buffer:
>  	kfree(buffer);
>  	return result;
>  }
>
> +A common type of bug to be aware of it "one err bugs" which look like this:
> +
> +err:
> +	kfree(foo->bar);
> +	kfree(foo);
> +	return ret;
> +
> +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:".
> +
> +
>  		Chapter 8: Commenting
>
>  Comments are good, but there is also a danger of over-commenting.  NEVER
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[patch] CodingStyle: add some more error handling guidelines

[Dan Carpenter]

I added a paragraph on choosing label names, and updated the example
code to use a better label name.  I also cleaned up the example code to
more modern style by moving the allocation out of the initializer and
changing the NULL check.

Perhaps the most common type of error handling bug in the kernel is "one
err bugs".  CodingStyle already says that we should "avoid nesting" by
using error labels and one err style error handling tends to have
multiple indent levels, so this was already bad style.  But I've added a
new paragraph explaining how to avoid one err bugs by using multiple
error labels which is, hopefully, more clear.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 9f28b14..9c8a234 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
 locations and some common work such as cleanup has to be done.  If there is no
 cleanup needed then just return directly.
 
-The rationale is:
+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:"
+
+The rationale for using gotos is:
 
 - unconditional statements are easier to understand and follow
 - nesting is reduced
@@ -403,9 +408,10 @@ The rationale is:
 int fun(int a)
 {
 	int result = 0;
-	char *buffer = kmalloc(SIZE);
+	char *buffer;
 
-	if (buffer == NULL)
+	buffer = kmalloc(SIZE);
+	if (!buffer)
 		return -ENOMEM;
 
 	if (condition1) {
@@ -413,14 +419,25 @@ int fun(int a)
 			...
 		}
 		result = 1;
-		goto out;
+		goto out_buffer;
 	}
 	...
-out:
+out_buffer:
 	kfree(buffer);
 	return result;
 }
 
+A common type of bug to be aware of it "one err bugs" which look like this:
+
+err:
+	kfree(foo->bar);
+	kfree(foo);
+	return ret;
+
+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:".
+
+
 		Chapter 8: Commenting
 
 Comments are good, but there is also a danger of over-commenting.  NEVER