diff mbox series

libkmod: fix result of comparison of unsigned enum in kmod_dump_index()

Message ID 20240622061321.9005-2-yskelg@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series libkmod: fix result of comparison of unsigned enum in kmod_dump_index() | expand

Commit Message

Yunseong Kim June 22, 2024, 6:13 a.m. UTC 
From: Yunseong Kim <yskelg@gmail.com>

This patch fix compiler warning in kmod_dump_index()

libkmod/libkmod.c:989:11: warning: result of comparison of unsigned enum
expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
  989 |         if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
      |             ~~~~ ^ ~

My compiler version.

$ clang -v
clang version 18.1.6 (Fedora 18.1.6-3.fc40)

Signed-off-by: Yunseong Kim <yskelg@gmail.com>
---
 libkmod/libkmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michal Suchánek June 24, 2024, 7:49 a.m. UTC | #1 
Hello,

On Sat, Jun 22, 2024 at 03:13:22PM +0900, yskelg@gmail.com wrote:
> From: Yunseong Kim <yskelg@gmail.com>
> 
> This patch fix compiler warning in kmod_dump_index()
> 
> libkmod/libkmod.c:989:11: warning: result of comparison of unsigned enum
> expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
>   989 |         if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
>       |             ~~~~ ^ ~
> 
> My compiler version.
> 
> $ clang -v
> clang version 18.1.6 (Fedora 18.1.6-3.fc40)

If you look eg. here
https://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99

you can see that it is not *guaranteed* that the enum is unsigend, and
consequently a value lower than zero can be passed in.

Consequently the bug is in your compiler issuing the warning. While in
that specific C implementation the value cannot be negative in general
it can.

Finally I see that _KMOD_INDEX_MODULES_SIZE is defined separately from
the enumeration itself, and would be prone to getting slale if the
enumeration is ever updated.

Making it part of the enumeration so it automatically gets the value of
last used index type incremented by one would be probably less
error-prone. In the kernel there is also a pattern of defining *_LAST as
an alias to the last used value in the enumeration, and then
_KMOD_INDEX_MODULES_SIZE coud be defined as this last value incremented
by 1.

Thanks

Michal

> 
> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
> ---
>  libkmod/libkmod.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
> index 213b424..fcb619b 100644
> --- a/libkmod/libkmod.c
> +++ b/libkmod/libkmod.c
> @@ -986,7 +986,7 @@ KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type,
>  	if (ctx == NULL)
>  		return -ENOSYS;
>  
> -	if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
> +	if (type >= KMOD_INDEX_MODULES_DEP || type >= _KMOD_INDEX_MODULES_SIZE)

Why are you adding a duplicate check here?

>  		return -ENOENT;
>  
>  	if (ctx->indexes[type] != NULL) {
> -- 
> 2.45.2
>
Yunseong Kim June 24, 2024, 5:40 p.m. UTC | #2 
Hi Michal,

On 6/24/24 4:49 오후, Michal Suchánek wrote:
> Hello,
> 
> On Sat, Jun 22, 2024 at 03:13:22PM +0900, yskelg@gmail.com wrote:
>> From: Yunseong Kim <yskelg@gmail.com>
>>
>> This patch fix compiler warning in kmod_dump_index()
>>
>> libkmod/libkmod.c:989:11: warning: result of comparison of unsigned enum
>> expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
>>   989 |         if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
>>       |             ~~~~ ^ ~
>>
>> My compiler version.
>>
>> $ clang -v
>> clang version 18.1.6 (Fedora 18.1.6-3.fc40)
> 
> If you look eg. here
> https://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99
> 
> you can see that it is not *guaranteed* that the enum is unsigend, and
> consequently a value lower than zero can be passed in.
> 
> Consequently the bug is in your compiler issuing the warning. While in
> that specific C implementation the value cannot be negative in general
> it can.
> 
> Finally I see that _KMOD_INDEX_MODULES_SIZE is defined separately from
> the enumeration itself, and would be prone to getting slale if the
> enumeration is ever updated.
> 
> Making it part of the enumeration so it automatically gets the value of
> last used index type incremented by one would be probably less
> error-prone. In the kernel there is also a pattern of defining *_LAST as
> an alias to the last used value in the enumeration, and then
> _KMOD_INDEX_MODULES_SIZE coud be defined as this last value incremented
> by 1.
> 
> Thanks
> 
> Michal

Thank you for the code review Michal, I understand your point. Would it
be acceptable if we don't need to make any further adjustments to the
warning?


>> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
>> ---
>>  libkmod/libkmod.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
>> index 213b424..fcb619b 100644
>> --- a/libkmod/libkmod.c
>> +++ b/libkmod/libkmod.c
>> @@ -986,7 +986,7 @@ KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type,
>>  	if (ctx == NULL)
>>  		return -ENOSYS;
>>  
>> -	if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
>> +	if (type >= KMOD_INDEX_MODULES_DEP || type >= _KMOD_INDEX_MODULES_SIZE)
> 
> Why are you adding a duplicate check here?

I also think my code is really wrong. I'm sorry.

>>  		return -ENOENT;
>>  
>>  	if (ctx->indexes[type] != NULL) {
>> -- 
>> 2.45.2
>>


Warm regards,

Yunseong Kim
Michal Suchánek June 24, 2024, 6:46 p.m. UTC | #3 
Hello,

On Tue, Jun 25, 2024 at 02:40:23AM +0900, Yunseong Kim wrote:
> Hi Michal,
> 
> On 6/24/24 4:49 오후, Michal Suchánek wrote:
> > Hello,
> > 
> > On Sat, Jun 22, 2024 at 03:13:22PM +0900, yskelg@gmail.com wrote:
> >> From: Yunseong Kim <yskelg@gmail.com>
> >>
> >> This patch fix compiler warning in kmod_dump_index()
> >>
> >> libkmod/libkmod.c:989:11: warning: result of comparison of unsigned enum
> >> expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
> >>   989 |         if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
> >>       |             ~~~~ ^ ~
> >>
> >> My compiler version.
> >>
> >> $ clang -v
> >> clang version 18.1.6 (Fedora 18.1.6-3.fc40)
> > 
> > If you look eg. here
> > https://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99
> > 
> > you can see that it is not *guaranteed* that the enum is unsigend, and
> > consequently a value lower than zero can be passed in.
> > 
> > Consequently the bug is in your compiler issuing the warning. While in
> > that specific C implementation the value cannot be negative in general
> > it can.
> > 
> > Finally I see that _KMOD_INDEX_MODULES_SIZE is defined separately from
> > the enumeration itself, and would be prone to getting slale if the
> > enumeration is ever updated.
> > 
> > Making it part of the enumeration so it automatically gets the value of
> > last used index type incremented by one would be probably less
> > error-prone. In the kernel there is also a pattern of defining *_LAST as
> > an alias to the last used value in the enumeration, and then
> > _KMOD_INDEX_MODULES_SIZE coud be defined as this last value incremented
> > by 1.
> > 
> > Thanks
> > 
> > Michal
> 
> Thank you for the code review Michal, I understand your point. Would it
> be acceptable if we don't need to make any further adjustments to the
> warning?

As already said the warning should be adjusted on the compiler side.
That is either report the problem to the compiler authors so they can
fix it or disable the warning with a compiler option if it's bothering
you.

> >> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
> >> index 213b424..fcb619b 100644
> >> --- a/libkmod/libkmod.c
> >> +++ b/libkmod/libkmod.c
> >> @@ -986,7 +986,7 @@ KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type,
> >>  	if (ctx == NULL)
> >>  		return -ENOSYS;
> >>  
> >> -	if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
> >> +	if (type >= KMOD_INDEX_MODULES_DEP || type >= _KMOD_INDEX_MODULES_SIZE)
> > 
> > Why are you adding a duplicate check here?
> 
> I also think my code is really wrong. I'm sorry.

These two are defined in different files, so it's understandable that
you might get confused about their relationship. Defining them in one
place could make the code more robust and easier to understand. It
would, however, not address the warning. It's a separate problem related
to this enum definition.

Thanks

Michal
Lucas De Marchi June 28, 2024, 10:08 p.m. UTC | #4 
On Mon, Jun 24, 2024 at 08:46:12PM GMT, Michal Suchánek wrote:
>Hello,
>
>On Tue, Jun 25, 2024 at 02:40:23AM +0900, Yunseong Kim wrote:
>> Hi Michal,
>>
>> On 6/24/24 4:49 오후, Michal Suchánek wrote:
>> > Hello,
>> >
>> > On Sat, Jun 22, 2024 at 03:13:22PM +0900, yskelg@gmail.com wrote:
>> >> From: Yunseong Kim <yskelg@gmail.com>
>> >>
>> >> This patch fix compiler warning in kmod_dump_index()
>> >>
>> >> libkmod/libkmod.c:989:11: warning: result of comparison of unsigned enum
>> >> expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
>> >>   989 |         if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
>> >>       |             ~~~~ ^ ~
>> >>
>> >> My compiler version.
>> >>
>> >> $ clang -v
>> >> clang version 18.1.6 (Fedora 18.1.6-3.fc40)
>> >
>> > If you look eg. here
>> > https://stackoverflow.com/questions/2579230/signedness-of-enum-in-c-c99-c-cx-gnu-c-gnu-c99
>> >
>> > you can see that it is not *guaranteed* that the enum is unsigend, and
>> > consequently a value lower than zero can be passed in.
>> >
>> > Consequently the bug is in your compiler issuing the warning. While in
>> > that specific C implementation the value cannot be negative in general
>> > it can.
>> >
>> > Finally I see that _KMOD_INDEX_MODULES_SIZE is defined separately from
>> > the enumeration itself, and would be prone to getting slale if the
>> > enumeration is ever updated.
>> >
>> > Making it part of the enumeration so it automatically gets the value of
>> > last used index type incremented by one would be probably less
>> > error-prone. In the kernel there is also a pattern of defining *_LAST as
>> > an alias to the last used value in the enumeration, and then
>> > _KMOD_INDEX_MODULES_SIZE coud be defined as this last value incremented
>> > by 1.
>> >
>> > Thanks
>> >
>> > Michal
>>
>> Thank you for the code review Michal, I understand your point. Would it
>> be acceptable if we don't need to make any further adjustments to the
>> warning?
>
>As already said the warning should be adjusted on the compiler side.
>That is either report the problem to the compiler authors so they can
>fix it or disable the warning with a compiler option if it's bothering
>you.
>
>> >> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
>> >> index 213b424..fcb619b 100644
>> >> --- a/libkmod/libkmod.c
>> >> +++ b/libkmod/libkmod.c
>> >> @@ -986,7 +986,7 @@ KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type,
>> >>  	if (ctx == NULL)
>> >>  		return -ENOSYS;
>> >>
>> >> -	if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
>> >> +	if (type >= KMOD_INDEX_MODULES_DEP || type >= _KMOD_INDEX_MODULES_SIZE)
>> >
>> > Why are you adding a duplicate check here?
>>
>> I also think my code is really wrong. I'm sorry.
>
>These two are defined in different files, so it's understandable that
>you might get confused about their relationship. Defining them in one
>place could make the code more robust and easier to understand. It
>would, however, not address the warning. It's a separate problem related
>to this enum definition.

yes, but the main issue is that libkmod/libkmod.h is the header exposed
to the users, making it harder to maintain API compatibility since the
value exposed would be changing.

Lucas De Marchi

>
>Thanks
>
>Michal
diff mbox series

Patch

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 213b424..fcb619b 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -986,7 +986,7 @@  KMOD_EXPORT int kmod_dump_index(struct kmod_ctx *ctx, enum kmod_index type,
 	if (ctx == NULL)
 		return -ENOSYS;
 
-	if (type < 0 || type >= _KMOD_INDEX_MODULES_SIZE)
+	if (type >= KMOD_INDEX_MODULES_DEP || type >= _KMOD_INDEX_MODULES_SIZE)
 		return -ENOENT;
 
 	if (ctx->indexes[type] != NULL) {