Hi Michal, On 6/24/24 4:49 오후, Michal Suchánek wrote: > Hello, > > On Sat, Jun 22, 2024 at 03:13:22PM +0900, yskelg@xxxxxxxxx wrote: >> From: Yunseong Kim <yskelg@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> --- >> 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
