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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160822213743-mutt-send-email-mst@kernel.org>
Date:   Mon, 22 Aug 2016 21:39:35 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
        Julia Lawall <julia.lawall@...6.fr>,
        Jason Wang <jasowang@...hat.com>, linux-doc@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH] CodingStyle: add some more error handling guidelines

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