lkml.org 
[lkml]   [2016]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/4] virtio_blk: Less function calls in init_vq() after error detection
    From
    Date
    On 09/13/2016 02:13 PM, SF Markus Elfring wrote:
    > From: Markus Elfring <elfring@users.sourceforge.net>
    > Date: Tue, 13 Sep 2016 13:20:44 +0200
    >
    > The kfree() function was called in up to three cases
    > by the init_vq() function during error handling even if
    > the passed variable contained a null pointer.
    >
    > * Split a condition check for memory allocation failures.
    >
    > * Adjust jump targets according to the Linux coding style convention.
    >
    > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
    > ---
    > drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
    > 1 file changed, 17 insertions(+), 5 deletions(-)

    Can't you see from this diffstat that the patch actually seems to makes
    the code more complex?
    In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
    virtio_blk: Fix a slient kernel panic

    which did the opposite of your patch. And in fact it fixed a bug. Quite obviously
    multiple labels are harder to read and harder to get right. For error handling with
    just kfree one label is just the right thing to.

    >
    > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
    > index 6553eb7..d28dbcf 100644
    > --- a/drivers/block/virtio_blk.c
    > +++ b/drivers/block/virtio_blk.c
    > @@ -395,11 +395,21 @@ static int init_vq(struct virtio_blk *vblk)
    > return -ENOMEM;
    >
    > names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL);
    > + if (!names) {
    > + err = -ENOMEM;
    > + goto free_vblk_vqs;
    > + }
    > +
    > callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL);
    > + if (!callbacks) {
    > + err = -ENOMEM;
    > + goto free_names;
    > + }
    > +
    > vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL);
    > - if (!names || !callbacks || !vqs) {
    > + if (!vqs) {
    > err = -ENOMEM;
    > - goto out;
    > + goto free_callbacks;
    > }
    >
    > for (i = 0; i < num_vqs; i++) {
    > @@ -411,19 +421,21 @@ static int init_vq(struct virtio_blk *vblk)
    > /* Discover virtqueues and write information to configuration. */
    > err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
    > if (err)
    > - goto out;
    > + goto free_vqs;
    >
    > for (i = 0; i < num_vqs; i++) {
    > spin_lock_init(&vblk->vqs[i].lock);
    > vblk->vqs[i].vq = vqs[i];
    > }
    > vblk->num_vqs = num_vqs;
    > -
    > -out:
    > + free_vqs:
    > kfree(vqs);
    > + free_callbacks:
    > kfree(callbacks);
    > + free_names:
    > kfree(names);
    > if (err)
    > + free_vblk_vqs:
    > kfree(vblk->vqs);
    > return err;
    > }
    >

    \
     
     \ /
      Last update: 2016-09-17 09:59    [W:2.448 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site