Messages in this thread | | | From | Nick Desaulniers <> | Date | Tue, 7 Jun 2022 13:29:30 -0700 | Subject | Re: [PATCH] netfilter: conntrack: fix this -Wformat clang warning: |
| |
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
| |