Re: [Outreachy kernel] [PATCH] Staging: lustre: lustre: fid: Remove unnecessary else after return

[Jes Sorensen <jes.sorensen@gmail.com>]

On 02/20/15 13:27, aybuke ozdemir wrote:
> This patch fixes the following checkpatch warning:
> WARNING : else is not generally useful after a break or return
> 204: FILE: drivers/staging/lustre/lustre/fid/fid_request.c
> 
> Signed-off-by: aybuke ozdemir <aybuke.147@gmail.com>
> ---
>  drivers/staging/lustre/lustre/fid/fid_request.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/fid/fid_request.c b/drivers/staging/lustre/lustre/fid/fid_request.c
> index 063441a..54dcfa3 100644
> --- a/drivers/staging/lustre/lustre/fid/fid_request.c
> +++ b/drivers/staging/lustre/lustre/fid/fid_request.c
> @@ -201,10 +201,9 @@ static int seq_client_alloc_seq(const struct lu_env *env,
>  			CERROR("%s: Can't allocate new meta-sequence, rc %d\n",
>  			       seq->lcs_name, rc);
>  			return rc;
> -		} else {
> -			CDEBUG(D_INFO, "%s: New range - "DRANGE"\n",
> -			       seq->lcs_name, PRANGE(&seq->lcs_space));
>  		}
> +		CDEBUG(D_INFO, "%s: New range - "DRANGE"\n",
> +			seq->lcs_name, PRANGE(&seq->lcs_space));
>  	} else {
>  		rc = 0;
>  	}
> 

The commit message would be better if you wrote a comment rather than
just copying in the checkpatch output.

Your solution is one way to resolve this issue, and technically correct.
However it's not always straight forward which it is the best solution.
In this particular case, I would consider an alternative avoiding the
'return' in the middle of the function, by using a goto. The rationale
for this is that if you have locks (mutex/spinlocks/etc) in the code it
makes it easier to track locking by reducing the number of exit points
from the code.

I would probably have chosen to make the code look like this (note this
is a hand-crafted pseudo patch):

static int seq_client_alloc_seq(const struct lu_env *env,
                                struct lu_client_seq *seq, u64 *seqnr)
{
        int rc;

        LASSERT(range_is_sane(&seq->lcs_space));

        if (range_is_exhausted(&seq->lcs_space)) {
                rc = seq_client_alloc_meta(env, seq);
                if (rc) {
                        CERROR("%s: Can't allocate new meta-sequence, rc
%d\n",
                               seq->lcs_name, rc);
-			return rc;
+                       goto out;
                } else {
                        CDEBUG(D_INFO, "%s: New range - "DRANGE"\n",
                               seq->lcs_name, PRANGE(&seq->lcs_space));
                }
        } else {
                rc = 0;
        }

        LASSERT(!range_is_exhausted(&seq->lcs_space));
        *seqnr = seq->lcs_space.lsr_start;
        seq->lcs_space.lsr_start += 1;

        CDEBUG(D_INFO, "%s: Allocated sequence [%#llx]\n", seq->lcs_name,
               *seqnr);

+ out:
        return rc;
}

A lot of this comes down to preference and judgment of the developer, as
there is more than one solution to the problem.

When you see an error message from checkpatch like this one, it is good
idea to take a step back and consider which solution makes the most sense.

Cheers,
Jes