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

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

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

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

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

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