* [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] 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 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 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 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] 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.