* [PATCH] netfilter: conntrack: fix this -Wformat clang warning: @ 2022-06-06 21:28 Justin Stitt 2022-06-07 15:27 ` Nathan Chancellor 0 siblings, 1 reply; 8+ messages in thread From: Justin Stitt @ 2022-06-06 21:28 UTC (permalink / raw) To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal Cc: llvm, Nick Desaulniers, Justin Stitt | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.tcp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.tcp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.udp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.udp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.dccp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.dccp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.sctp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.sctp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Variadic functions (printf-like) undergo default argument promotion. Documentation/core-api/printk-formats.rst specifically recommends using the promoted-to-type's format flag. Also, as per C11 6.3.1.1: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) `If an int can represent all values of the original type ..., the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.` Thus it makes sense to change %hu (as well as %u) to %d. Signed-off-by: Justin Stitt <jstitt007@gmail.com> --- net/netfilter/nf_conntrack_standalone.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c index 6ad7bbc90d38..afbec8a12c5e 100644 --- a/net/netfilter/nf_conntrack_standalone.c +++ b/net/netfilter/nf_conntrack_standalone.c @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, switch (l4proto->l4proto) { case IPPROTO_ICMP: - seq_printf(s, "type=%u code=%u id=%u ", + seq_printf(s, "type=%d code=%d id=%d ", tuple->dst.u.icmp.type, tuple->dst.u.icmp.code, ntohs(tuple->src.u.icmp.id)); break; case IPPROTO_TCP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.tcp.port), ntohs(tuple->dst.u.tcp.port)); break; case IPPROTO_UDPLITE: case IPPROTO_UDP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.udp.port), ntohs(tuple->dst.u.udp.port)); break; case IPPROTO_DCCP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.dccp.port), ntohs(tuple->dst.u.dccp.port)); break; case IPPROTO_SCTP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.sctp.port), ntohs(tuple->dst.u.sctp.port)); break; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: conntrack: fix this -Wformat clang warning: 2022-06-06 21:28 [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Justin Stitt @ 2022-06-07 15:27 ` Nathan Chancellor 2022-06-07 18:08 ` [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() Justin Stitt 2022-06-07 20:29 ` [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Nick Desaulniers 0 siblings, 2 replies; 8+ messages in thread From: Nathan Chancellor @ 2022-06-07 15:27 UTC (permalink / raw) To: Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, Nick Desaulniers Hi Justin, The subject might be a little more descriptive if if were something like: netfilter: conntrack: Fix clang -Wformat in print_tuple() rather than saying "this warning", as subjects should be standalone from the rest of the message. Minor nit but some maintainers have high standards for commit messages. On Mon, Jun 06, 2022 at 02:28:19PM -0700, Justin Stitt wrote: It would be helpful to note here that this warning appears when building arm64 with clang using -Wformat; the architecture is critical for reproducing this warning. > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.tcp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.tcp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.udp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.udp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.dccp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.dccp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.sctp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.sctp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Variadic functions (printf-like) undergo default argument promotion. > Documentation/core-api/printk-formats.rst specifically recommends > using the promoted-to-type's format flag. > > Also, as per C11 6.3.1.1: > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > `If an int can represent all values of the original type ..., the > value is converted to an int; otherwise, it is converted to an > unsigned int. These are called the integer promotions.` I think that it is probably worth noting that ntohs() has type 'int' in this case because arm64 does not define CONFIG_ARCH_USE_BUILTIN_BSWAP to use the version of __swab16() with a cast to __u16; rather, it uses the one with the conditional operator, which per 6.5.15.5 means the type of the result of the conditional operator is determined through these promotion rules as well. I suspect that the conditional versions of __swab{16,32,64}() in include/uapi/linux/swab.h want a cast on the outside of the conditional operator with the specified type but I am not sure what other repurcussions that might have so it is probably safer to just fix these warnings up one by one. > Thus it makes sense to change %hu (as well as %u) to %d. Regardless of that, changing from '%hu' to '%d' is recommended so that part of the change is fine. I am not sure that changing '%u' to '%d' does anything other than add to the diff, as there was no warnings from those, but consistency is key. > Signed-off-by: Justin Stitt <jstitt007@gmail.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > net/netfilter/nf_conntrack_standalone.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c > index 6ad7bbc90d38..afbec8a12c5e 100644 > --- a/net/netfilter/nf_conntrack_standalone.c > +++ b/net/netfilter/nf_conntrack_standalone.c > @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, > > switch (l4proto->l4proto) { > case IPPROTO_ICMP: > - seq_printf(s, "type=%u code=%u id=%u ", > + seq_printf(s, "type=%d code=%d id=%d ", > tuple->dst.u.icmp.type, > tuple->dst.u.icmp.code, > ntohs(tuple->src.u.icmp.id)); > break; > case IPPROTO_TCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.tcp.port), > ntohs(tuple->dst.u.tcp.port)); > break; > case IPPROTO_UDPLITE: > case IPPROTO_UDP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.udp.port), > ntohs(tuple->dst.u.udp.port)); > > break; > case IPPROTO_DCCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.dccp.port), > ntohs(tuple->dst.u.dccp.port)); > break; > case IPPROTO_SCTP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.sctp.port), > ntohs(tuple->dst.u.sctp.port)); > break; > -- > 2.30.2 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() 2022-06-07 15:27 ` Nathan Chancellor @ 2022-06-07 18:08 ` Justin Stitt 2022-06-07 20:33 ` Nick Desaulniers 2022-06-07 20:35 ` Nathan Chancellor 2022-06-07 20:29 ` [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Nick Desaulniers 1 sibling, 2 replies; 8+ messages in thread From: Justin Stitt @ 2022-06-07 18:08 UTC (permalink / raw) To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal Cc: llvm, Nick Desaulniers, Nathan Chancellor, Justin Stitt | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.tcp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.tcp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.udp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.udp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.dccp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.dccp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->src.u.sctp.port), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type | 'unsigned short' but the argument has type 'int' [-Wformat] | ntohs(tuple->dst.u.sctp.port)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Variadic functions (printf-like) undergo default argument promotion. Documentation/core-api/printk-formats.rst specifically recommends using the promoted-to-type's format flag. Also, as per C11 6.3.1.1: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) `If an int can represent all values of the original type ..., the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions.` Thus it makes sense to change %hu (as well as %u) to %d. It should be noted that %u does not produce the same warning as %hu in this context. However, it should probably be changed as well for consistency. Signed-off-by: Justin Stitt <jstitt007@gmail.com> --- Diff between v1 -> v2: * update commit message and subject line Note: The architecture (arm64) is critical for reproducing this warning. net/netfilter/nf_conntrack_standalone.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c index 6ad7bbc90d38..afbec8a12c5e 100644 --- a/net/netfilter/nf_conntrack_standalone.c +++ b/net/netfilter/nf_conntrack_standalone.c @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, switch (l4proto->l4proto) { case IPPROTO_ICMP: - seq_printf(s, "type=%u code=%u id=%u ", + seq_printf(s, "type=%d code=%d id=%d ", tuple->dst.u.icmp.type, tuple->dst.u.icmp.code, ntohs(tuple->src.u.icmp.id)); break; case IPPROTO_TCP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.tcp.port), ntohs(tuple->dst.u.tcp.port)); break; case IPPROTO_UDPLITE: case IPPROTO_UDP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.udp.port), ntohs(tuple->dst.u.udp.port)); break; case IPPROTO_DCCP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.dccp.port), ntohs(tuple->dst.u.dccp.port)); break; case IPPROTO_SCTP: - seq_printf(s, "sport=%hu dport=%hu ", + seq_printf(s, "sport=%d dport=%d ", ntohs(tuple->src.u.sctp.port), ntohs(tuple->dst.u.sctp.port)); break; -- 2.30.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() 2022-06-07 18:08 ` [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() Justin Stitt @ 2022-06-07 20:33 ` Nick Desaulniers 2022-06-07 20:36 ` Nick Desaulniers 2022-06-07 20:35 ` Nathan Chancellor 1 sibling, 1 reply; 8+ messages in thread From: Nick Desaulniers @ 2022-06-07 20:33 UTC (permalink / raw) To: Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, Nathan Chancellor On Tue, Jun 7, 2022 at 11:09 AM Justin Stitt <jstitt007@gmail.com> wrote: > > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.tcp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.tcp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.udp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.udp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.dccp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.dccp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.sctp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.sctp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Variadic functions (printf-like) undergo default argument promotion. > Documentation/core-api/printk-formats.rst specifically recommends > using the promoted-to-type's format flag. > > Also, as per C11 6.3.1.1: > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > `If an int can represent all values of the original type ..., the > value is converted to an int; otherwise, it is converted to an > unsigned int. These are called the integer promotions.` > Thus it makes sense to change %hu (as well as %u) to %d. > > It should be noted that %u does not produce the same warning as %hu in this > context. However, it should probably be changed as well for consistency. Right, because they are `unsigned char` and the parameter is unnamed for variadic functions they are also default-argument-promoted to int. -Wformat won't warn on signedness. Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Link: https://github.com/ClangBuiltLinux/linux/issues/378 Also, Nathan supplied his RB tag on v1; it's ok next time to include it on subsequent revisions of patches, so long as you don't change the patch too much between revisions. > > Signed-off-by: Justin Stitt <jstitt007@gmail.com> > --- > Diff between v1 -> v2: > * update commit message and subject line > > Note: The architecture (arm64) is critical for reproducing this warning. > > net/netfilter/nf_conntrack_standalone.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c > index 6ad7bbc90d38..afbec8a12c5e 100644 > --- a/net/netfilter/nf_conntrack_standalone.c > +++ b/net/netfilter/nf_conntrack_standalone.c > @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, > > switch (l4proto->l4proto) { > case IPPROTO_ICMP: > - seq_printf(s, "type=%u code=%u id=%u ", > + seq_printf(s, "type=%d code=%d id=%d ", > tuple->dst.u.icmp.type, > tuple->dst.u.icmp.code, > ntohs(tuple->src.u.icmp.id)); > break; > case IPPROTO_TCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.tcp.port), > ntohs(tuple->dst.u.tcp.port)); > break; > case IPPROTO_UDPLITE: > case IPPROTO_UDP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.udp.port), > ntohs(tuple->dst.u.udp.port)); > > break; > case IPPROTO_DCCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.dccp.port), > ntohs(tuple->dst.u.dccp.port)); > break; > case IPPROTO_SCTP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.sctp.port), > ntohs(tuple->dst.u.sctp.port)); > break; > -- > 2.30.2 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() 2022-06-07 20:33 ` Nick Desaulniers @ 2022-06-07 20:36 ` Nick Desaulniers 0 siblings, 0 replies; 8+ messages in thread From: Nick Desaulniers @ 2022-06-07 20:36 UTC (permalink / raw) To: Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, Nathan Chancellor, David S. Miller, Eric Dumazet, Jakub Kicinski, pabeni, Tom Rix, netfilter-devel, coreteam, Network Development, LKML Also, please make sure to run scripts/get_maintainer.pl on your patch file and CC everyone it recommends, with the maintainers you want to pick up your patch in the To field. https://lore.kernel.org/llvm/20220607180847.13482-1-jstitt007@gmail.com/T/#u $ ./scripts/get_maintainer.pl 0001-netfilter-conntrack-Fix-clang-Wformat-warning-in-pri.patch Pablo Neira Ayuso <pablo@netfilter.org> (maintainer:NETFILTER) Jozsef Kadlecsik <kadlec@netfilter.org> (maintainer:NETFILTER) Florian Westphal <fw@strlen.de> (maintainer:NETFILTER) "David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING [GENERAL]) Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING [GENERAL]) Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING [GENERAL]) Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING [GENERAL]) Nathan Chancellor <nathan@kernel.org> (supporter:CLANG/LLVM BUILD SUPPORT) Nick Desaulniers <ndesaulniers@google.com> (supporter:CLANG/LLVM BUILD SUPPORT) Tom Rix <trix@redhat.com> (reviewer:CLANG/LLVM BUILD SUPPORT) netfilter-devel@vger.kernel.org (open list:NETFILTER) coreteam@netfilter.org (open list:NETFILTER) netdev@vger.kernel.org (open list:NETWORKING [GENERAL]) linux-kernel@vger.kernel.org (open list) llvm@lists.linux.dev (open list:CLANG/LLVM BUILD SUPPORT On Tue, Jun 7, 2022 at 1:33 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Tue, Jun 7, 2022 at 11:09 AM Justin Stitt <jstitt007@gmail.com> wrote: > > > > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.tcp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.tcp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.udp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.udp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.dccp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.dccp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.sctp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.sctp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Variadic functions (printf-like) undergo default argument promotion. > > Documentation/core-api/printk-formats.rst specifically recommends > > using the promoted-to-type's format flag. > > > > Also, as per C11 6.3.1.1: > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > > `If an int can represent all values of the original type ..., the > > value is converted to an int; otherwise, it is converted to an > > unsigned int. These are called the integer promotions.` > > Thus it makes sense to change %hu (as well as %u) to %d. > > > > It should be noted that %u does not produce the same warning as %hu in this > > context. However, it should probably be changed as well for consistency. > > Right, because they are `unsigned char` and the parameter is unnamed > for variadic functions they are also default-argument-promoted to int. > -Wformat won't warn on signedness. > > Thanks for the patch! > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > > Also, Nathan supplied his RB tag on v1; it's ok next time to include > it on subsequent revisions of patches, so long as you don't change the > patch too much between revisions. > > > > > Signed-off-by: Justin Stitt <jstitt007@gmail.com> > > --- > > Diff between v1 -> v2: > > * update commit message and subject line > > > > Note: The architecture (arm64) is critical for reproducing this warning. > > > > net/netfilter/nf_conntrack_standalone.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c > > index 6ad7bbc90d38..afbec8a12c5e 100644 > > --- a/net/netfilter/nf_conntrack_standalone.c > > +++ b/net/netfilter/nf_conntrack_standalone.c > > @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, > > > > switch (l4proto->l4proto) { > > case IPPROTO_ICMP: > > - seq_printf(s, "type=%u code=%u id=%u ", > > + seq_printf(s, "type=%d code=%d id=%d ", > > tuple->dst.u.icmp.type, > > tuple->dst.u.icmp.code, > > ntohs(tuple->src.u.icmp.id)); > > break; > > case IPPROTO_TCP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.tcp.port), > > ntohs(tuple->dst.u.tcp.port)); > > break; > > case IPPROTO_UDPLITE: > > case IPPROTO_UDP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.udp.port), > > ntohs(tuple->dst.u.udp.port)); > > > > break; > > case IPPROTO_DCCP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.dccp.port), > > ntohs(tuple->dst.u.dccp.port)); > > break; > > case IPPROTO_SCTP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.sctp.port), > > ntohs(tuple->dst.u.sctp.port)); > > break; > > -- > > 2.30.2 > > > > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() 2022-06-07 18:08 ` [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() Justin Stitt 2022-06-07 20:33 ` Nick Desaulniers @ 2022-06-07 20:35 ` Nathan Chancellor 1 sibling, 0 replies; 8+ messages in thread From: Nathan Chancellor @ 2022-06-07 20:35 UTC (permalink / raw) To: Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, Nick Desaulniers On Tue, Jun 07, 2022 at 11:08:47AM -0700, Justin Stitt wrote: > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.tcp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.tcp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.udp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.udp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.dccp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.dccp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->src.u.sctp.port), > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > | 'unsigned short' but the argument has type 'int' [-Wformat] > | ntohs(tuple->dst.u.sctp.port)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Variadic functions (printf-like) undergo default argument promotion. > Documentation/core-api/printk-formats.rst specifically recommends > using the promoted-to-type's format flag. > > Also, as per C11 6.3.1.1: > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > `If an int can represent all values of the original type ..., the > value is converted to an int; otherwise, it is converted to an > unsigned int. These are called the integer promotions.` > Thus it makes sense to change %hu (as well as %u) to %d. > > It should be noted that %u does not produce the same warning as %hu in this > context. However, it should probably be changed as well for consistency. > > Signed-off-by: Justin Stitt <jstitt007@gmail.com> Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > Diff between v1 -> v2: > * update commit message and subject line > > Note: The architecture (arm64) is critical for reproducing this warning. > > net/netfilter/nf_conntrack_standalone.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c > index 6ad7bbc90d38..afbec8a12c5e 100644 > --- a/net/netfilter/nf_conntrack_standalone.c > +++ b/net/netfilter/nf_conntrack_standalone.c > @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, > > switch (l4proto->l4proto) { > case IPPROTO_ICMP: > - seq_printf(s, "type=%u code=%u id=%u ", > + seq_printf(s, "type=%d code=%d id=%d ", > tuple->dst.u.icmp.type, > tuple->dst.u.icmp.code, > ntohs(tuple->src.u.icmp.id)); > break; > case IPPROTO_TCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.tcp.port), > ntohs(tuple->dst.u.tcp.port)); > break; > case IPPROTO_UDPLITE: > case IPPROTO_UDP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.udp.port), > ntohs(tuple->dst.u.udp.port)); > > break; > case IPPROTO_DCCP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.dccp.port), > ntohs(tuple->dst.u.dccp.port)); > break; > case IPPROTO_SCTP: > - seq_printf(s, "sport=%hu dport=%hu ", > + seq_printf(s, "sport=%d dport=%d ", > ntohs(tuple->src.u.sctp.port), > ntohs(tuple->dst.u.sctp.port)); > break; > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: conntrack: fix this -Wformat clang warning: 2022-06-07 15:27 ` Nathan Chancellor 2022-06-07 18:08 ` [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() Justin Stitt @ 2022-06-07 20:29 ` Nick Desaulniers 2022-06-07 20:46 ` Nathan Chancellor 1 sibling, 1 reply; 8+ messages in thread From: Nick Desaulniers @ 2022-06-07 20:29 UTC (permalink / raw) To: Nathan Chancellor, Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, LKML On Tue, Jun 7, 2022 at 8:27 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Hi Justin, > > The subject might be a little more descriptive if if were something > like: > > netfilter: conntrack: Fix clang -Wformat in print_tuple() > > rather than saying "this warning", as subjects should be standalone from > the rest of the message. Minor nit but some maintainers have high > standards for commit messages. > > On Mon, Jun 06, 2022 at 02:28:19PM -0700, Justin Stitt wrote: > > It would be helpful to note here that this warning appears when building > arm64 with clang using -Wformat; the architecture is critical for > reproducing this warning. > > > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.tcp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.tcp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.udp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.udp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.dccp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.dccp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->src.u.sctp.port), > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > | ntohs(tuple->dst.u.sctp.port)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Variadic functions (printf-like) undergo default argument promotion. > > Documentation/core-api/printk-formats.rst specifically recommends > > using the promoted-to-type's format flag. > > > > Also, as per C11 6.3.1.1: > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > > `If an int can represent all values of the original type ..., the > > value is converted to an int; otherwise, it is converted to an > > unsigned int. These are called the integer promotions.` > > I think that it is probably worth noting that ntohs() has type 'int' in > this case because arm64 does not define CONFIG_ARCH_USE_BUILTIN_BSWAP to > use the version of __swab16() with a cast to __u16; rather, it uses the > one with the conditional operator, which per 6.5.15.5 means the type of > the result of the conditional operator is determined through these > promotion rules as well. > > I suspect that the conditional versions of __swab{16,32,64}() in > include/uapi/linux/swab.h want a cast on the outside of the conditional > operator with the specified type but I am not sure what other > repurcussions that might have so it is probably safer to just fix these > warnings up one by one. Yes, there's a subtly where for ternary's the resulting type is the largest of all operands. When the LHS of the ? is larger than either results, the result is STILL the larger type. https://godbolt.org/z/EGjW8sqzf That said, regardless of that change, unnamed parameters of variadic functions will still undergo default argument promotion, so the __u16 would still be promoted to int. So we still want this patch, too. Justin, can you send a separate patch adding the explicit cast to to __u16 for the ternary expression in __swab16 when __HAVE_BUILTIN_BSWAP16__ is not defined? include/uapi/linux/swab.h Please include Nathan's suggested by tag; cc me and I'll provide review. > > > Thus it makes sense to change %hu (as well as %u) to %d. > > Regardless of that, changing from '%hu' to '%d' is recommended so that > part of the change is fine. I am not sure that changing '%u' to '%d' > does anything other than add to the diff, as there was no warnings from > those, but consistency is key. These types in the case of icmp are uint8_t; they also are default-argument-promoted to int. All possible values of uint8_t are representable by int, and so %d and %u will make no difference. It is "no functional change" and simply being consistent, as you say. > > > Signed-off-by: Justin Stitt <jstitt007@gmail.com> > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > > --- > > net/netfilter/nf_conntrack_standalone.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c > > index 6ad7bbc90d38..afbec8a12c5e 100644 > > --- a/net/netfilter/nf_conntrack_standalone.c > > +++ b/net/netfilter/nf_conntrack_standalone.c > > @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple, > > > > switch (l4proto->l4proto) { > > case IPPROTO_ICMP: > > - seq_printf(s, "type=%u code=%u id=%u ", > > + seq_printf(s, "type=%d code=%d id=%d ", > > tuple->dst.u.icmp.type, > > tuple->dst.u.icmp.code, > > ntohs(tuple->src.u.icmp.id)); > > break; > > case IPPROTO_TCP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.tcp.port), > > ntohs(tuple->dst.u.tcp.port)); > > break; > > case IPPROTO_UDPLITE: > > case IPPROTO_UDP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.udp.port), > > ntohs(tuple->dst.u.udp.port)); > > > > break; > > case IPPROTO_DCCP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.dccp.port), > > ntohs(tuple->dst.u.dccp.port)); > > break; > > case IPPROTO_SCTP: > > - seq_printf(s, "sport=%hu dport=%hu ", > > + seq_printf(s, "sport=%d dport=%d ", > > ntohs(tuple->src.u.sctp.port), > > ntohs(tuple->dst.u.sctp.port)); > > break; > > -- > > 2.30.2 > > > > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: conntrack: fix this -Wformat clang warning: 2022-06-07 20:29 ` [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Nick Desaulniers @ 2022-06-07 20:46 ` Nathan Chancellor 0 siblings, 0 replies; 8+ messages in thread From: Nathan Chancellor @ 2022-06-07 20:46 UTC (permalink / raw) To: Nick Desaulniers Cc: Justin Stitt, Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, llvm, LKML On Tue, Jun 07, 2022 at 01:29:30PM -0700, Nick Desaulniers wrote: > On Tue, Jun 7, 2022 at 8:27 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > Hi Justin, > > > > The subject might be a little more descriptive if if were something > > like: > > > > netfilter: conntrack: Fix clang -Wformat in print_tuple() > > > > rather than saying "this warning", as subjects should be standalone from > > the rest of the message. Minor nit but some maintainers have high > > standards for commit messages. > > > > On Mon, Jun 06, 2022 at 02:28:19PM -0700, Justin Stitt wrote: > > > > It would be helpful to note here that this warning appears when building > > arm64 with clang using -Wformat; the architecture is critical for > > reproducing this warning. > > > > > | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->src.u.tcp.port), > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->dst.u.tcp.port)); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->src.u.udp.port), > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->dst.u.udp.port)); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->src.u.dccp.port), > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->dst.u.dccp.port)); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->src.u.sctp.port), > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type > > > | 'unsigned short' but the argument has type 'int' [-Wformat] > > > | ntohs(tuple->dst.u.sctp.port)); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > Variadic functions (printf-like) undergo default argument promotion. > > > Documentation/core-api/printk-formats.rst specifically recommends > > > using the promoted-to-type's format flag. > > > > > > Also, as per C11 6.3.1.1: > > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf) > > > `If an int can represent all values of the original type ..., the > > > value is converted to an int; otherwise, it is converted to an > > > unsigned int. These are called the integer promotions.` > > > > I think that it is probably worth noting that ntohs() has type 'int' in > > this case because arm64 does not define CONFIG_ARCH_USE_BUILTIN_BSWAP to > > use the version of __swab16() with a cast to __u16; rather, it uses the > > one with the conditional operator, which per 6.5.15.5 means the type of > > the result of the conditional operator is determined through these > > promotion rules as well. > > > > I suspect that the conditional versions of __swab{16,32,64}() in > > include/uapi/linux/swab.h want a cast on the outside of the conditional > > operator with the specified type but I am not sure what other > > repurcussions that might have so it is probably safer to just fix these > > warnings up one by one. > > Yes, there's a subtly where for ternary's the resulting type is the > largest of all operands. When the LHS of the ? is larger than either > results, the result is STILL the larger type. > https://godbolt.org/z/EGjW8sqzf Ha, I used the same test locally to make sure I was understanding things correctly :) > That said, regardless of that change, unnamed parameters of variadic > functions will still undergo default argument promotion, so the __u16 > would still be promoted to int. So we still want this patch, too. Ah right, I forgot about the default argument promotion rules. While the result of ntohs() would still be promoted to int (and we should use the wider specifier for that reason as you said), we wouldn't see a warning in the first place because the type of ntohs() would match the specifier in all cases, rather than !CONFIG_ARCH_USE_BUILTIN_BSWAP, which is the only reason I made that comment. I suspect adding that cast will clear up more warnings than just this one, depending on how many there are from ntohs() (and __swab16()). > Justin, can you send a separate patch adding the explicit cast to to > __u16 for the ternary expression in __swab16 when > __HAVE_BUILTIN_BSWAP16__ is not defined? > include/uapi/linux/swab.h > Please include Nathan's suggested by tag; cc me and I'll provide review. > > > > > > Thus it makes sense to change %hu (as well as %u) to %d. > > > > Regardless of that, changing from '%hu' to '%d' is recommended so that > > part of the change is fine. I am not sure that changing '%u' to '%d' > > does anything other than add to the diff, as there was no warnings from > > those, but consistency is key. > > These types in the case of icmp are uint8_t; they also are > default-argument-promoted to int. All possible values of uint8_t are > representable by int, and so %d and %u will make no difference. It is > "no functional change" and simply being consistent, as you say. Right, I should have made that more clear by adding ", so that part is fine as well", thanks for spelling it out! Cheers, Nathan ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-07 20:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-06 21:28 [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Justin Stitt 2022-06-07 15:27 ` Nathan Chancellor 2022-06-07 18:08 ` [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple() Justin Stitt 2022-06-07 20:33 ` Nick Desaulniers 2022-06-07 20:36 ` Nick Desaulniers 2022-06-07 20:35 ` Nathan Chancellor 2022-06-07 20:29 ` [PATCH] netfilter: conntrack: fix this -Wformat clang warning: Nick Desaulniers 2022-06-07 20:46 ` Nathan Chancellor
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.