Re: [PATCH 08/12] staging: wilc1000: rename variable s32Error

[Julian Calaby <julian.calaby@gmail.com>]

Hi Leo,

On Thu, Jan 28, 2016 at 6:13 PM, Leo Kim <leo.kim@atmel.com> wrote:
> This patch renames variable s32Error to result
> to avoid CamelCase naming convention.
> Also, remove the unused variable s32Error and replace with direct return.
>
> Signed-off-by: Leo Kim <leo.kim@atmel.com>
> ---
>  drivers/staging/wilc1000/coreconfigurator.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/coreconfigurator.c b/drivers/staging/wilc1000/coreconfigurator.c
> index 2a4e324..88e5661 100644
> --- a/drivers/staging/wilc1000/coreconfigurator.c
> +++ b/drivers/staging/wilc1000/coreconfigurator.c
> @@ -356,30 +356,29 @@ s32 wilc_parse_network_info(u8 *msg_buffer, tstrNetworkInfo **ret_network_info)
>
>  s32 wilc_dealloc_network_info(tstrNetworkInfo *pstrNetworkInfo)
>  {
> -       s32 s32Error = 0;
> +       s32 result = 0;
>
>         if (pstrNetworkInfo) {
>                 if (pstrNetworkInfo->pu8IEs) {
>                         kfree(pstrNetworkInfo->pu8IEs);
>                         pstrNetworkInfo->pu8IEs = NULL;

I'm not sure it's necessary to set this to NULL here.

>                 } else {
> -                       s32Error = -EFAULT;
> +                       result = -EFAULT;
>                 }
>
>                 kfree(pstrNetworkInfo);
>                 pstrNetworkInfo = NULL;

Or this to NULL here.

Also, is the return value from this function actually checked? kfree()
can have NULL passed to it.

If the return value isn't checked, this entire function could be simplified to:

- - - - -

if (pstrNetworkInfo) {
    kfree(pstrNetworkInfo->pu8IEs);
    kfree(pstrNetworkInfo);
}

return 0;

- - - - -

Also, when tstrNetworkInfo structs are allocated, wouldn't the
allocating function fail if ->pu8IEs is NULL?

>         } else {
> -               s32Error = -EFAULT;
> +               result = -EFAULT;
>         }
>
> -       return s32Error;
> +       return result;
>  }
>
>  s32 wilc_parse_assoc_resp_info(u8 *pu8Buffer, u32 u32BufferLen,
>                                tstrConnectRespInfo **ppstrConnectRespInfo)
>  {
> -       s32 s32Error = 0;
>         tstrConnectRespInfo *pstrConnectRespInfo = NULL;
>         u16 u16AssocRespLen = 0;
>         u8 *pu8IEs = NULL;
> @@ -408,27 +407,27 @@ s32 wilc_parse_assoc_resp_info(u8 *pu8Buffer, u32 u32BufferLen,
>
>         *ppstrConnectRespInfo = pstrConnectRespInfo;
>
> -       return s32Error;
> +       return 0;
>  }
>
>  s32 wilc_dealloc_assoc_resp_info(tstrConnectRespInfo *pstrConnectRespInfo)
>  {
> -       s32 s32Error = 0;
> +       s32 result = 0;

All the comments above apply to this function as well.

>
>         if (pstrConnectRespInfo) {
>                 if (pstrConnectRespInfo->pu8RespIEs) {
>                         kfree(pstrConnectRespInfo->pu8RespIEs);
>                         pstrConnectRespInfo->pu8RespIEs = NULL;
>                 } else {
> -                       s32Error = -EFAULT;
> +                       result = -EFAULT;
>                 }
>
>                 kfree(pstrConnectRespInfo);
>                 pstrConnectRespInfo = NULL;
>
>         } else {
> -               s32Error = -EFAULT;
> +               result = -EFAULT;
>         }
>
> -       return s32Error;
> +       return result;
>  }

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/