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