[PATCH] Eliminate extra 'out_free' label from fcoe_init function

[PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
---
 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);
 out_destroy:
 	destroy_workqueue(fcoe_wq);

[PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
---
 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);
 out_destroy:
 	destroy_workqueue(fcoe_wq);

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Johannes Thumshirn]

On 06/01/2017 02:11 PM, 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 <mgandhi@redhat.com>
> ---

Ahm and what happens to the fcoe_wq then?


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Johannes Thumshirn]

On 06/01/2017 02:11 PM, 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 <mgandhi@redhat.com>
> ---

Ahm and what happens to the fcoe_wq then?


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Dan Carpenter]

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 <mgandhi@redhat.com>
> ---
>  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

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Dan Carpenter]

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 <mgandhi@redhat.com>
> ---
>  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

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
>> ---
>>  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.

Thanks,
Milan.


Thanks,
Milan.

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
>> ---
>>  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.

Thanks,
Milan.


Thanks,
Milan.

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Julia Lawall]

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 <mgandhi@redhat.com>
> >> ---
> >>  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

>
> Thanks,
> Milan.
>
>
> Thanks,
> Milan.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Julia Lawall]

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 <mgandhi@redhat.com>
> >> ---
> >>  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

>
> Thanks,
> Milan.
>
>
> Thanks,
> Milan.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
>>>> ---
>>>>  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.

Re: [PATCH] Eliminate extra 'out_free' label from fcoe_init function

[Milan P. Gandhi]

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 <mgandhi@redhat.com>
>>>> ---
>>>>  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.