Search Linux Wireless

Re: [PATCH 1/2] staging: wilc1000: return zero on success and non-zero on function failure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,

On 27/01/20 1:07 pm, Dan Carpenter wrote:
> 
> Looks good.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> On Thu, Jan 23, 2020 at 12:50:47PM +0000, Ajay.Kathat@xxxxxxxxxxxxx wrote:
>> @@ -384,19 +378,18 @@ static int wilc_sdio_write_reg(struct wilc *wilc, u32 addr, u32 data)
>>               cmd.address = addr;
>>               cmd.data = data;
>>               ret = wilc_sdio_cmd52(wilc, &cmd);
>> -             if (ret) {
>> +             if (ret)
>>                       dev_err(&func->dev,
>>                               "Failed cmd 52, read reg (%08x) ...\n", addr);
>> -                     goto fail;
>> -             }
> 
> Please don't resend, but try to avoid this sort of anti-pattern in the
> future.  You're treating the last failure condition in the function as
> special.  In this case it's particularly difficult to read because we
> are far from the bottom of the function, but even if we were right at
> the bottom, I would try to avoid it.
> 
> I am extreme enough that I would avoid even things like:
> 
>         ret = frob();
>         if (ret)
>                 printf("ret failed\n");
>         return ret;
> 
> Instead I would write:
> 
>         ret = frob();
>         if (ret) {
>                 printf("ret failed\n");
>                 return ret;
>         }
>         return 0;
> 
> It's just nice to have the error path at indent level 2 and the
> success path at indent level 1.  And the other thing that I like is the
> BIG BOLD "return 0;" at the end of the function.  Some functions return
> positive numbers on success so if I see "return result;" then I have to
> look back a few lines to see if "result" can be positive.
> 
> The other anti-pattern which people often do is success handling
> (instead of error handling) for the last error condition in a function.
> 
>         ret = one();
>         if (ret)
>                 return ret;
>         ret = two();
>         if (ret)
>                 return ret;
>         ret = three();
>         if (!ret)
>                 ret = four();
>         return ret;
> 

Thanks for your useful advice to avoid anti-pattern. I shall keep these
points in mind while working on future patches.

Regards,
Ajay




[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux