On 06/02/2017 11:19 AM, Julia Lawall wrote: > > > On Fri, 2 Jun 2017, Milan P. Gandhi wrote: > >> On 06/01/2017 08:32 PM, Dan Carpenter wrote: >>> On Thu, Jun 01, 2017 at 05:41:06PM +0530, Milan P. Gandhi wrote: >>>> Simplify the check for return code of fcoe_if_init routine >>>> in fcoe_init function such that we could eliminate need for >>>> extra 'out_free' label. >>>> >>>> Signed-off-by: Milan P. Gandhi <[email protected]> >>>> --- >>>> drivers/scsi/fcoe/fcoe.c | 10 ++++------ >>>> 1 file changed, 4 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c >>>> index ea21e7b..fb2a4c9 100644 >>>> --- a/drivers/scsi/fcoe/fcoe.c >>>> +++ b/drivers/scsi/fcoe/fcoe.c >>>> @@ -2523,13 +2523,11 @@ static int __init fcoe_init(void) >>>> fcoe_dev_setup(); >>>> >>>> rc = fcoe_if_init(); >>>> - if (rc) >>>> - goto out_free; >>>> - >>>> - mutex_unlock(&fcoe_config_mutex); >>>> - return 0; >>>> + if (rc == 0) { >>>> + mutex_unlock(&fcoe_config_mutex); >>>> + return 0; >>>> + } >>>> >>>> -out_free: >>>> mutex_unlock(&fcoe_config_mutex); >>> >>> Gar... Stop! No1 Don't do this. >>> >>> Do failure handling, not success handling. >>> >>> People always think they should get creative with the last if statement >>> in a function. This leads to spaghetti code and it's confusing. Please >>> never do this again. >>> >>> The original is correct and the new code is bad rubbish code. >>> >>> regards, >>> dan carpenter >>> >>> >> >> Oops, my bad sir. Will keep this in mind. > > Still, does the mutex_unlock really need to be duplicated? > > julia >
Hello Julia, Thanks for your hint! I have found a better way to remove a need for duplicate mutex_unlock statement and extra 'out_free' label. Will send the corrected path for the same. Many thanks, Milan.
