lkml.org 
[lkml]   [2017]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] staging: rtl8188eu: Fix private WEXT IOCTL calls
    On Sat, Nov 25, 2017 at 02:29:36AM +0100, ishraq.i.ashraf@gmail.com wrote:
    > +
    > + ret = 0;
    > +

    Sorry, I wasn't clear before. When I said don't initialize "ret" to
    zero, I just meant that in that specific case we initialized "ret" and
    then immediately reassigned it with:

    ret = some_function();
    if (ret)
    return ret;

    In this case it's fine to set "ret = 0" at the start so that we don't
    have to do it later.

    > + if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
    > + ret = -EFAULT;
    > +
    > + if (pwep)
    > + goto err_free_pwep_param;
    > +
    > + err_free_param:
    > + kfree(param);
    > + return ret;
    > +
    > + err_free_pwep_param:
    > + kfree(pwep);
    > + kfree(param);
    > + return ret;
    > +}

    Hm... I said before that it's better to keep the error paths and
    success path separate but in this case it's probabaly simpler to merge
    them.

    This one could look like this:

    if (copy_to_user(wrqu->data.pointer, param, wrqu->data.length))
    ret = -EFAULT;

    free_pwep:
    kfree(pwep);
    free_param:
    kfree(param);
    return ret;

    There is no need for the if (pwep) conditions, because kfree() can
    take a NULL pointer. Some people would just use one label but I hate
    that. It looks like this:

    free:
    kfree(pwep);
    kfree(param);
    return ret;

    The reason, I hate it is because I don't like freeing things which have
    not been allocated yet. If you do it the normal kernel way then you
    just have to keep track of the most recently allocated thing.

    regards,
    dan carpenter

    \
     
     \ /
      Last update: 2017-11-25 05:56    [W:4.598 / U:0.228 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site