diff mbox series

[v2] ocfs2: Reomve err less than zero check

Message ID 1620643062-124920-1-git-send-email-jiapeng.chong@linux.alibaba.com (mailing list archive)
State New
Headers show
Series [v2] ocfs2: Reomve err less than zero check | expand

Commit Message

Jiapeng Chong May 10, 2021, 10:37 a.m. UTC
Because enum dlm_status starts from 0, the check for err < 0 is always
false.

Clean up the following coccicheck warning:

fs/ocfs2/dlm/dlmdebug.c:222 dlm_errname() warn: unsigned 'err' is never
less than zero.

fs/ocfs2/dlm/dlmdebug.c:214 dlm_errmsg() warn: unsigned 'err' is never
less than zero.

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
Changes in v2:
  -Update commit log.

 fs/ocfs2/dlm/dlmdebug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Joseph Qi May 10, 2021, 11:33 a.m. UTC | #1
On 5/10/21 6:37 PM, Jiapeng Chong wrote:
> Because enum dlm_status starts from 0, the check for err < 0 is always
> false.
> 
> Clean up the following coccicheck warning:
> 
> fs/ocfs2/dlm/dlmdebug.c:222 dlm_errname() warn: unsigned 'err' is never
> less than zero.
> 
> fs/ocfs2/dlm/dlmdebug.c:214 dlm_errmsg() warn: unsigned 'err' is never
> less than zero.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>

Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> ---
> Changes in v2:
>   -Update commit log.
> 
>  fs/ocfs2/dlm/dlmdebug.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index d442cf5..817914d 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -211,7 +211,7 @@ void dlm_print_one_lock(struct dlm_lock *lockid)
>  
>  const char *dlm_errmsg(enum dlm_status err)
>  {
> -	if (err >= DLM_MAXSTATS || err < 0)
> +	if (err >= DLM_MAXSTATS)
>  		return dlm_errmsgs[DLM_MAXSTATS];
>  	return dlm_errmsgs[err];
>  }
> @@ -219,7 +219,7 @@ const char *dlm_errmsg(enum dlm_status err)
>  
>  const char *dlm_errname(enum dlm_status err)
>  {
> -	if (err >= DLM_MAXSTATS || err < 0)
> +	if (err >= DLM_MAXSTATS)
>  		return dlm_errnames[DLM_MAXSTATS];
>  	return dlm_errnames[err];
>  }
>
Wengang Wang May 10, 2021, 3:50 p.m. UTC | #2
Hi Jiapeng,

Though the type of enum dlm_status has a value range starting from zero,  It can be assigned with negative values without warnings.
I am wondering why you are sure it can’t be negative?  You went over all the calling path and so you are sure it can’t be negative?
If you did that, I’d think/guess you should also be able to say it can’t be DLM_MAXSTATS or bigger, right? If not, which calling path are returning >= DLM_MAXSTATS value?
we should fix that too.

I’d think we should treat the two conditions of (err >= DLM_MAXSTATS) and the (err < 0) the same way.  If keep, keep both. if remove, remove both.
I’m suspecting the coccicheck warning is based on the type of enum (starting with 0), but if that’s the only theory to remove (err < 0), It’s NAK from me.

thanks,
wengang

> On May 10, 2021, at 3:37 AM, Jiapeng Chong <jiapeng.chong@linux.alibaba.com> wrote:
> 
> Because enum dlm_status starts from 0, the check for err < 0 is always
> false.
> 
> Clean up the following coccicheck warning:
> 
> fs/ocfs2/dlm/dlmdebug.c:222 dlm_errname() warn: unsigned 'err' is never
> less than zero.
> 
> fs/ocfs2/dlm/dlmdebug.c:214 dlm_errmsg() warn: unsigned 'err' is never
> less than zero.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> ---
> Changes in v2:
>  -Update commit log.
> 
> fs/ocfs2/dlm/dlmdebug.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index d442cf5..817914d 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -211,7 +211,7 @@ void dlm_print_one_lock(struct dlm_lock *lockid)
> 
> const char *dlm_errmsg(enum dlm_status err)
> {
> -	if (err >= DLM_MAXSTATS || err < 0)
> +	if (err >= DLM_MAXSTATS)
> 		return dlm_errmsgs[DLM_MAXSTATS];
> 	return dlm_errmsgs[err];
> }
> @@ -219,7 +219,7 @@ const char *dlm_errmsg(enum dlm_status err)
> 
> const char *dlm_errname(enum dlm_status err)
> {
> -	if (err >= DLM_MAXSTATS || err < 0)
> +	if (err >= DLM_MAXSTATS)
> 		return dlm_errnames[DLM_MAXSTATS];
> 	return dlm_errnames[err];
> }
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
Joseph Qi May 11, 2021, 2:16 a.m. UTC | #3
On 5/10/21 11:50 PM, Wengang Wang wrote:
> Hi Jiapeng,
> 
> Though the type of enum dlm_status has a value range starting from zero,  It can be assigned with negative values without warnings.
> I am wondering why you are sure it can’t be negative?  You went over all the calling path and so you are sure it can’t be negative?
> If you did that, I’d think/guess you should also be able to say it can’t be DLM_MAXSTATS or bigger, right? If not, which calling path are returning >= DLM_MAXSTATS value?
> we should fix that too.
> 
> I’d think we should treat the two conditions of (err >= DLM_MAXSTATS) and the (err < 0) the same way.  If keep, keep both. if remove, remove both.
> I’m suspecting the coccicheck warning is based on the type of enum (starting with 0), but if that’s the only theory to remove (err < 0), It’s NAK from me
DLM_MAXSTATS is a valid value for dlm_status.
Take a look again, I agree with Wengang that we'd better keep the check
here for those misused return code.

Thanks,
Joseph
diff mbox series

Patch

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index d442cf5..817914d 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -211,7 +211,7 @@  void dlm_print_one_lock(struct dlm_lock *lockid)
 
 const char *dlm_errmsg(enum dlm_status err)
 {
-	if (err >= DLM_MAXSTATS || err < 0)
+	if (err >= DLM_MAXSTATS)
 		return dlm_errmsgs[DLM_MAXSTATS];
 	return dlm_errmsgs[err];
 }
@@ -219,7 +219,7 @@  const char *dlm_errmsg(enum dlm_status err)
 
 const char *dlm_errname(enum dlm_status err)
 {
-	if (err >= DLM_MAXSTATS || err < 0)
+	if (err >= DLM_MAXSTATS)
 		return dlm_errnames[DLM_MAXSTATS];
 	return dlm_errnames[err];
 }