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

Dan Carpenter
2016-08-22T18:31:47Z

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