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@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? 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
