| Message ID | 20230522175401.1232921-1-dario.binacchi@amarulasolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | [ethtool,v2,1/1] netlink/rss: move variable declaration out of the for loop | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, May 22, 2023 at 07:54:01PM +0200, Dario Binacchi wrote: > The patch fixes this compilation error: > > netlink/rss.c: In function 'rss_reply_cb': > netlink/rss.c:166:3: error: 'for' loop initial declarations are only allowed in C99 mode > for (unsigned int i = 0; i < get_count(hash_funcs); i++) { > ^ > netlink/rss.c:166:3: note: use option -std=c99 or -std=gnu99 to compile your code > > The project doesn't really need a C99 compiler, so let's move the variable > declaration outside the for loop. > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > --- To be honest, I'm rather surprised that this would be the only C99 feature in ethtool code, I thought that e.g. the named struct initializers also require C99. Anyway, with kernel explicitly declaring C11 as the standard to use since 5.18, it would IMHO make more sense to do the same in ethtool so that developers do not need to keep in mind that they cannot use language features they are used to from kernel. What do you think? Michal
Hi Michal, On Tue, May 23, 2023 at 7:06 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > On Mon, May 22, 2023 at 07:54:01PM +0200, Dario Binacchi wrote: > > The patch fixes this compilation error: > > > > netlink/rss.c: In function 'rss_reply_cb': > > netlink/rss.c:166:3: error: 'for' loop initial declarations are only allowed in C99 mode > > for (unsigned int i = 0; i < get_count(hash_funcs); i++) { > > ^ > > netlink/rss.c:166:3: note: use option -std=c99 or -std=gnu99 to compile your code > > > > The project doesn't really need a C99 compiler, so let's move the variable > > declaration outside the for loop. > > > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > To be honest, I'm rather surprised that this would be the only C99 > feature in ethtool code, I thought that e.g. the named struct > initializers also require C99. Yes, you're probably right. That might be the first of the compilation errors, or in other cases, the compiler I used may only display warnings. > > Anyway, with kernel explicitly declaring C11 as the standard to use > since 5.18, it would IMHO make more sense to do the same in ethtool so > that developers do not need to keep in mind that they cannot use > language features they are used to from kernel. What do you think? I agree with you. Anyway, to fix this issue https://patchwork.ozlabs.org/project/buildroot/patch/20230520211246.3950131-1-dario.binacchi@amarulasolutions.com/ can I send patch updating configure.ac as suggested by Yann E. Morin ? --- a/configure.ac +++ b/configure.ac @@ -12,6 +12,10 @@ AC_PROG_CC AC_PROG_GCC_TRADITIONAL AM_PROG_CC_C_O PKG_PROG_PKG_CONFIG +AC_PROG_CC_C99 +AS_IF([test "x$ac_cv_prog_cc_c99" = "xno"], [ + AC_MSG_ERROR([no C99 compiler found, $PACKAGE requires a C99 compiler.]) +]) Thanks and regards, Dario > > Michal
On Tue, May 23, 2023 at 09:28:19AM +0200, Dario Binacchi wrote: > On Tue, May 23, 2023 at 7:06 AM Michal Kubecek <mkubecek@suse.cz> wrote: > > Anyway, with kernel explicitly declaring C11 as the standard to use > > since 5.18, it would IMHO make more sense to do the same in ethtool so > > that developers do not need to keep in mind that they cannot use > > language features they are used to from kernel. What do you think? > > I agree with you. > Anyway, to fix this issue > https://patchwork.ozlabs.org/project/buildroot/patch/20230520211246.3950131-1-dario.binacchi@amarulasolutions.com/ > can I send patch updating configure.ac as suggested by Yann E. Morin ? > > --- a/configure.ac > +++ b/configure.ac > @@ -12,6 +12,10 @@ AC_PROG_CC > AC_PROG_GCC_TRADITIONAL > AM_PROG_CC_C_O > PKG_PROG_PKG_CONFIG > +AC_PROG_CC_C99 > +AS_IF([test "x$ac_cv_prog_cc_c99" = "xno"], [ > + AC_MSG_ERROR([no C99 compiler found, $PACKAGE requires a C99 compiler.]) > +]) I like this approach much more than the previous patch. But how about using C11 instead? My point is that kernel is already using -std=c11 (or, more precisely, -std=gnu11) for build for about a year and we can expect significant overlap between kernel and ethtool developers. Therefore choosing C99 would inevitably result in similar issues in the future (people using features they are used to from kernel and breaking the build on older systems not having C11 as default). As far as I can see, GCC should support -std=c11 since version 4.7, that does not seem to be too restricting dependency to me. Michal
diff --git a/netlink/rss.c b/netlink/rss.c index 4ad6065ef698..a31c10aeca95 100644 --- a/netlink/rss.c +++ b/netlink/rss.c @@ -93,6 +93,7 @@ int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data) bool silent; int err_ret; int ret; + unsigned int i; silent = nlctx->is_dump || nlctx->is_monitor; err_ret = silent ? MNL_CB_OK : MNL_CB_ERROR; @@ -163,7 +164,7 @@ int rss_reply_cb(const struct nlmsghdr *nlhdr, void *data) printf(" Operation not supported\n"); return 0; } - for (unsigned int i = 0; i < get_count(hash_funcs); i++) { + for (i = 0; i < get_count(hash_funcs); i++) { printf(" %s: %s\n", get_string(hash_funcs, i), (rss_hfunc & (1 << i)) ? "on" : "off"); }
The patch fixes this compilation error: netlink/rss.c: In function 'rss_reply_cb': netlink/rss.c:166:3: error: 'for' loop initial declarations are only allowed in C99 mode for (unsigned int i = 0; i < get_count(hash_funcs); i++) { ^ netlink/rss.c:166:3: note: use option -std=c99 or -std=gnu99 to compile your code The project doesn't really need a C99 compiler, so let's move the variable declaration outside the for loop. Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- Changes in v2: - Change 'int' to 'unsigned int'. --- netlink/rss.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)